meilisearch / meilisearch-php

PHP wrapper for the Meilisearch API
https://meilisearch.com
MIT License
569 stars 95 forks source link

DX - Make SDK classes `final` when possible #518

Open stloyd opened 1 year ago

stloyd commented 1 year ago

Description

I would even suggest locking all the classes with the final keyword, to prevent the BC breaks, and allow easier maintenance and a safer Developer Experience in the end.

In case we may think that it's worth introducing an interface covering SDK classes to allow either mocking/stubbing, but that could require additional work on standardizing the classes and responsibilities.

Refs: https://ocramius.github.io/blog/when-to-declare-classes-final/

mmachatschek commented 1 year ago

Concerning the final topic. I hear a lot of negatives about this (in the Laravel community) to not use this feature, even though it looks like a good idea. Users that need to extend classes marked as final will need to wait for upstream or have to fork libs themselves if they run into issues. Also there are workarounds to extend classes even if they are marked as final so there is no 100% guarantee that the class marked as final will be in the final state.

https://github.com/meilisearch/meilisearch-php/pull/514#issuecomment-1571596226

brunoocasali commented 1 year ago

The PHP code will probably not break, and the change is good. But since it could lead to unexpected results like the one pointed out by @mmachatschek I would prefer to do it in a v2 time.

norkunas commented 1 year ago

@final phpdoc could be added to indicate that it will become final in v2