-
Notifications
You must be signed in to change notification settings - Fork 231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Basic implementation of psr6 compatibility pass #690
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When reviewing this PR, please make sure to have a look over my comments as well, as I'm sure they all raise some potential points for (beneficial) discussion / constructive criticism.
Thanks @Ahummeling! In order to fix the build, can you please execute The failing test should be fixed if you rebase with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left an initial review, great work so far!
{ | ||
private const CACHE_SETTER_METHODS_PSR6_SUPPORT = [ | ||
'setMetadataCache' => true, | ||
'setMetadataCacheImpl' => true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'setMetadataCacheImpl' => true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
'doctrine/doctrine-bundle', | ||
'2.4', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'doctrine/doctrine-bundle', | |
'2.4', | |
'doctrine/mongodb-odm-bundle', | |
'4.4', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, fixed that
just need to have a testcase added for the compiler pass now and this should be complete |
$compatibilityLayerId = $definitionId . '.compatibility_layer'; | ||
$container->setAlias($compatibilityLayerId, $aliasId); | ||
|
||
$container->setDefinition($compatibilityLayerId, $compatibilityLayer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this, but if I had understood correctly, if $compatibilityLayer
is not null
, it is a definition implementing PSR-6 to replace the current definition, so should we replace the current alias with the compatibilityLayer one (flip arguments)?:
$container->setAlias($aliasId, $compatibilityLayerId);
c439111
to
b53bf56
Compare
@Ahummeling I've rebased, tried to add tests for the compiler pass (based on the ORM) and fixed some psalm issues, finally I've flipped the alias arguments to make tests pass, but not sure if I was right about that last one 🤔 cc @alcaeus |
9836db1
to
7b77663
Compare
052ff4d
to
9103365
Compare
I've rebased, resolved conflicts and added a dummy assertion to make PHPUnit report the coverage for that CompilerPass test. |
Not sure why doesn't infer correctly that class string type 😕, maybe because of generics. |
Thanks, it looks good now! @alcaeus this is ready for review now. |
Thank you @Ahummeling! 🚀 |
After meeting with @alcaeus we came to the conclusion that it would be best to first implement psr6 compatibility, before moving forward with removing the metadata cache configuration. This pull request is aimed at implementing this compatibility, through means of a compiler pass.
Wanted to get a tiny bit of feedback as I am continuing to figure out which tests cause deprecation triggers, figure out what happens when I install this version in 1 of my projects and all that stuff.
Will be leaving some comments on my first commit to explain certain choices or things that might appear odd