-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[5.4] Plugin and events: extend deprecation to 7 #45818
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
Conversation
I think the phpstans error can be ignored here, because here is no real code changes, only comments. |
Hmm, no, they would require to change the baseline file, as the deprecation doc block comments are changed with this PR, so they cause new messages for PHPstan. |
I see, so need to edit them in baseline file, okay |
@Fedik Not edit. Just run |
Hm, not sure what is wrong with phpstan, on local run it does not show the error after update of baseline file |
@Fedik I've just pushed a fix for that, and if that works I will also do it for your other PR. |
@Fedik P.S.: WHen running PHPstan it is important that the branch is clean and up to date. Clean means there are no composer dependencies from previous runs of |
@Fedik P.P.S.: If you now check the changes of the phpstan-baseline.neon file in your PR you will see that there are no new entries, and only the version in the message has changed from 6 to 7. Maybe you can add this information to the testing instructions for reviewers. |
@richard67 thanks! |
@Fedik Weird, for your other PR I have that problem, PHPstan work locally but fails on GitHub. Maybe something with cached results of phpstan? I will continue to check. |
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.
LVGTM (looks VERY good to me).
Only the removal version in deprecation comments and messages have changed from 6 to 7, and in some cases comments have been improved or to do comments have been added.
The changes in the phpstan-baseline.md file are correct. They only reflect the changes in the reomval version, no ned entries have been added.
I think it is a good thing to have correct deprecation comments and messages already in 5.4.
Final check before merge: Scrolled through file changes, including |
Thank you @Fedik for your contribution. |
@Fedik Do you know if we need some documentation on manual.joomla.org for this PR? If so: Could you make a PR there? |
Yes, I just added it for both PRs joomla/Manual#493 |
Summary of Changes
Extend Events related deprecation till Joomla 7.
Testing Instructions
Code review
Link to documentations
Please select: