Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Symfony 7 support continuation #175
Symfony 7 support continuation #175
Changes from 4 commits
61db250
1a77d6f
696f314
8b59bb9
65ba8cc
7fa167e
c962988
ef7807d
2bc1c75
654921f
6e8b590
a141bda
0a593c0
17934de
7cc1b2a
00da055
16455ae
dfd83ed
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
But I also think we should drop support for Symfony 3.4 and 4.4 too. Those are officially unmaintained anyway, so it's time to drop it. And I think we can bump 5 & 6 to 5.4 & 6.4 - I hope it will help us to make tests green easier
So, the ideal Symfony support version constraints should be:
^5.4 || ^6.4 || ^7.0
here and in other places.Wdyt?
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.
Yes, I also thought something (see remark in PR message) similar, but I thought I'd await your opinion about this since you have more in depth knowledge of the repository.
I'll update the PR.
UPDATE: Done.
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.
Ok, I managed to fix some of the tests, however the other ones I'm not really sure what to do with. For the the remaining errors and failures it's related to my remark in the PR message. I'll provide an example:
In some of the code, the used annotations are in the middle of variables
But if you try to put an attribute there it will give syntax errors. Furthermore, attributes only seem to be allowed in front of class/function definitions and class properties.
I'm not aware of any workarounds for this, so I'm not really sure how one would want to support this kind of functionality using attributes. Any of you have an idea?
EDIT:
I noticed the PHP STAN and CSFixer tests were failing because of an older PHP version, so I added ^7.3 ("back") to composer.
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.
I see, that's a bummer :/
I'm not sure. Probably we can use class properties where we will use the attribute and then reuse the property in that code? I'm not sure if this will stop working because of this though.
Or I wonder if we can continue using PHP annotations in this specific spot maybe?
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.
I agree, that felt like an obvious thing to try, but sadly that didn't work. About using 2 separate readers, I'm guessing that's probably possible.
Also did not think about the fact that adding 7.3 back does not allow typing of class properties, so I'm not sure what you would prefer? Want me to remove the typing or bump it to 8.*?
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.
Thank you! ❤️
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.
I think I have some bad news. I think I found what you were talking about.
I was reverting the changes, and when I came across this file
tests/Functional/Visitor/Php/Symfony/ValidationAnnotationTest.php
I was unable to change it back to AnnotationLoader without PHPstorm complaining.Some furthing digging dug up:
soooo.... what now? :)
EDIT:
Ok, so I've just put back the AttributeLoader again and for those cases I am able to use the attributes (It's not actually this packages custom attributes, but rather a translation inside a validator attribute):
The final error it's failing on now is
\Translation\Extractor\Tests\Functional\Visitor\Php\Symfony\ValidationAnnotationTest::testExtractAnnotationError
Which seems weird, because you would expect this to error, but for some reason it does not:
(I just replaced the annotation with its attribute counterpart)
Maybe rewrite the test that it does not check for errors but just to check if it did not find any valid attributes? When I dump the collection variable inside the test it just has two empty arrays:
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.
Ah, heck... that's exactly the problem I faced, now I remember it :/
So, I'm not sure about the best solution here, I'm open to ideas
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.
:)
Perhaps a way to test this is to change the test from
to
Since apparently the AttributeReader does not give in errors when putting in non-existing attributes to extract. It rather just results in no attributes.
I'd like to hear what you think about this change. I've committed and pushed it so you can look over the changes without me copy pasting just snippets. To be clear, this commit passes the local
vendor/bin/simple-phpunit
testsuite.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.
Ok so I've had some pressing other issues to take care of but this evening I had some spare time to look into this PR.
I figured out how to run the GitHub actions locally so I was able to fix most of the warnings and errors that the statistical tests threw. However I've got one left that I don't really understand, and I was hoping it makes more sense to you:
I was able to fix a similar error by adding a null check in the code. I tried the same here, but no luck. However a notable difference is that the other error did indicate a line number where it occurred, and this one does not. Also this one includes a part ("was not matched in reported errors") that the other did not.
EDIT:
Minor change and it seems to be passing the last test too now.
EDIT2: The package I'm using it in required a bump of a dependency of this package (
nikic/php-parser
) so that it could also use5.0.0
. This caused some issues but they are fixed too. All tests pass locally, so fingers crossed and it should work now.