Open zluiten opened 3 years ago
Thanks @netiul for raising the issue. Your detailed analysis is awesome!
I'd be totally OK if we go the BC break way and introduce the next version to only support PHP 8 including attributes only. I don't think the current version would run on PHP 8 since we rely on ProxyManager which comes with some very strict PHP version constraints. If the next version is 0.11 or 0.20 or maybe 0.5 to signal the larger change, I don't favor anything.
Since we can't simply upgrade doctrine/annotations and have attribute support out of the box, I would recommend dropping doctrine/annotations support completely and "just" focus on the attributes. I would want to support both ways at the same time as this introduces not needed complexity.
From my point of view, we would need to upgrade all the dependencies first to support PHP 8 and adapt to possible changes in code plus get some basic work done like run the builds on GitHub actions and ideally add psalm & phpstan checks in the mix. There might be changes in ProxyManager that we would need to adapt to. Once we have that working we could start removing doctrine/annotations and change the code to support PHP 8's attributes. I like your attributes syntax proposal, that looks fine for me.
Hey @heiglandreas any thoughts you want to put into this?
The ProxyManager upgrade is mentioned in #130 already.
Thanks @netiul for raising the issue. Your detailed analysis is awesome!
Thanks for being positive :)
I don't think the current version would run on PHP 8 since we rely on ProxyManager which comes with some very strict PHP version constraints.
:+1: Yes, looks like we are blocked by ProxyManager until it supports PHP 8. Besides that, a version of ProxyManager supporting both 7.4 and 8.0 might not be very likely.
I would want to support both ways at the same time as this introduces not needed complexity.
I think you mean "I would not want to support..."? Anyway I agree I guess, supporting both annotations and attributes at the same time might not be worth it.
From my point of view, we would need to upgrade all the dependencies first to support PHP 8 and adapt to possible changes in code plus get some basic work done like run the builds on GitHub actions and ideally add psalm & phpstan checks in the mix.
Sounds good. I noticed dependencies being out of date. PHP 7.4 is the only 7.x release series not being EOL and it's not even in the test pipeline. I would suggest (besides your suggestions) to bump the minimum supported PHP version to 7.4 in the next minor release. I can make a start if that's okay.
There might be changes in ProxyManager that we would need to adapt to. Once we have that working we could start removing doctrine/annotations and change the code to support PHP 8's attributes.
:+1:
Correct, I meant there is no need to support both Annotations and Attributes at the same time.
For ProxyManager there's a PR open to achieve PHP 8 compatibility: https://github.com/Ocramius/ProxyManager/pull/628
Upping the minimum requirements to 7.4 and having all dependencies in the latest possible version sounds like a good start to me. Once we have that we could try to run Disco with PHP 8 and see if ProxyManager breaks things or not. I could imagine that things could work when we don't use PHP 8 features like union types.
Just came across this package, maybe it can help us: https://github.com/mbunge/php-attributes
Marco released ProxyManager 2.11.0 which supports PHP 8: https://github.com/Ocramius/ProxyManager/releases/tag/2.11.0
Upgrade to PHP 7.4 done via #132. Next step: Upgrade to PHP 8.0
@netiul master branch is PHP 8.0 only now. Do you want to tackle the move to PHP's Attributes now?
@shochdoerfer I definitely would like to help with that move, but I'm caught up in work for a couple more weeks. So if anyone else wants to pick up the slack in the mean time, go ahead... 😄
@netiul I guess you are still busy with other stuff?
Does not look like anyone wants to drive this forward anymore ;(
There's a new kid on the block that might be interesting for those of you looking for a PHP8 version of Disco that makes use of attributes: https://github.com/giuseppe998e/syringe
With PHP 8 and having attributes support, configuring beans can be even more simpler. I don't expect to have a complete picture of what's needed, since I have only experience with discolight where I did this proposal (sort of). Anyway here's my take:
There are some options to walk this path:
I can't think of a good reason to go for option 2. Adding it to 0.x allows for a gradual upgrade path allowing to deprecate features which can eventually be removed in 1.x.
Adding this feature can be done by:
While new Attribute classes allows you to start with a clean slate, current Annotation classes can quite easily be adapted in order to be used as Attribute classes as well.
As option 1 is in my opinion the most valuable approach I'll go a bit further on that:
Add support to current Annotation classes
Example of how that can be achieved in case of the Alias class:
As you can see named arguments allow for a clean usage of existing Annotation classes.
What about nested attributes? In PHP's current implementation of attributes nesting is not supported, so explicitly configuring needs to be done on the same level, or instances of nested attributes/annotations should be able to be configured with scalars and instantiated in the "parent" attribute instance.
What does this means for each type of annotation?
Alias Allow configuration of
Bean Allow configuration of
BeanPostProcessor Allow configuration of
Configuration Just an identifier attribute
Parameter The ordering of parameters is important since the method is called with the parameters in the order they are configured. Nesting the parameters inside the Bean annotation provides a natural way of respecting the ordering of the arguments in the methods signature. But requiring attributes to be listed in a specific order feels a bit like a smell to me.
Possible solution: When using the Parameter attribute, require a config key with the name of the argument so it can be used for calling the method with named arguments. The nicest would be something like this:
Unfortunately name is already in use as the name of the parameter key, but i.e. argname can perhaps be a good alternative.
How to incorporate this with the ConfigurationGenerator?
The doctrine AnnotationReader can be swapped out for a FallbackAttributeReader. The FallbackAttributeReader would prioritize attributes over Annotations.
I would not recommend mixing annotations and attributes on the same level.
Possible guidelines: