Fix PHP 8.4 deprecations and add some typing #26
Conversation
| @@ -23,7 +23,7 @@ class TraceableChainActivator extends ChainActivator | |||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
| public function isActive($name, Context $context): bool | |||
| public function isActive($name, ?Context $context): bool | |||
There was a problem hiding this comment.
@calderholding-r why does the $context need to be nullable? Wouldn't it have thrown an exception if a null was provided before?
There was a problem hiding this comment.
Hi @yakobe,
It's not strange you'd expect an exception, but no. There is some history which leads to this confusing situation.
You'll find a proper explanation at https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullable-deprecated and https://wiki.php.net/rfc/deprecate-implicitly-nullable-types.
So prior to PHP 8.4 this was a bit more flexible, but this new version deprecates this in favor of a more logically and expected way of typing parameters and obviously fits within the path PHP is taking towards more strict typing.
There was a problem hiding this comment.
After re-reading your question I should clarify: if you call the method with null as argument it will indeed fail. The deprecation relates to the declaration itself.
There was a problem hiding this comment.
Sorry, I meant that it did not allow null before, so it shouldn't need to allow it now either. I don't think this change is needed. If it was Context $context = null before then it would be needed.
There was a problem hiding this comment.
Now I do understand what you mean. I think you are right, but I'm not sure without testing it and I won't be able to spend any time on this before later next week. If you are right we could do with changing the FeatureManager(Interface) hierarchy and calls in the sdk and keeping the FeatureActivatorInterface hierarchy and calls as they were on the master branch. In the FeatureManager there is always a default Context provided if the seconds parameter is null, so it's kind of silly to have both an empty Context and to make it nullable. Mind you, in the PhpDoc block it does say 'optional', so maybe a bit of documenting changes wouldn't be the worst idea. :) What shall we do? Either you look into this to verify and to make the changes it or it has to wait until I have the time. The changes in FeatureDataCollector and the sdk are definitely needed, so the PR won't be a total waste. ;)
Hi,
Following playox/flagception-sdk#8, this is a pull request to add PHP 8.4 support to Symfony flagception-bundle.
I was able to run all the sdk tests, but I wasn't able to run some of the tests in this repository (db flags, etc.). If you would please recheck these, that would be appreciated.