Skip to content

[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

Merged
merged 8 commits into from
Aug 9, 2025

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Aug 2, 2025

Summary of Changes

Extend Events related deprecation till Joomla 7.

Testing Instructions

Code review

Link to documentations

Please select:

@Fedik
Copy link
Member Author

Fedik commented Aug 2, 2025

I think the phpstans error can be ignored here, because here is no real code changes, only comments.

@richard67
Copy link
Member

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.

@Fedik
Copy link
Member Author

Fedik commented Aug 2, 2025

I see, so need to edit them in baseline file, okay

@richard67
Copy link
Member

I see, so need to edit them in baseline file, okay

@Fedik Not edit. Just run ./libraries/vendor/bin/phpstan -b to recreate the baseline file, and ./libraries/vendor/bin/phpstan -vv to check the result (of course all after a composer install):

@Fedik
Copy link
Member Author

Fedik commented Aug 3, 2025

Hm, not sure what is wrong with phpstan, on local run it does not show the error after update of baseline file

@richard67
Copy link
Member

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.

@richard67
Copy link
Member

@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 composer install for other branches. To be sure of that, I always do a git clean -d -x -f and then a git checkout . to get rid of any changes, then maybe a git pull to be sure the local branch is up to date with the remove, then run composer installand then you can use phpstan.

@richard67
Copy link
Member

richard67 commented Aug 3, 2025

@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.

@Fedik
Copy link
Member Author

Fedik commented Aug 3, 2025

@richard67 thanks!
I tried on "up to date" branch, but maybe still were something missed

@richard67
Copy link
Member

@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.

Copy link
Member

@richard67 richard67 left a 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.

@muhme
Copy link
Contributor

muhme commented Aug 9, 2025

Final check before merge: Scrolled through file changes, including phpstan-baseline.neon

@muhme muhme merged commit 18fcf37 into joomla:5.4-dev Aug 9, 2025
40 checks passed
@muhme muhme added this to the Joomla! 5.4.0 milestone Aug 9, 2025
@muhme
Copy link
Contributor

muhme commented Aug 9, 2025

Thank you @Fedik for your contribution.

@Fedik Fedik deleted the plugin-events-extend-deprecation7 branch August 9, 2025 19:51
@richard67
Copy link
Member

@Fedik Do you know if we need some documentation on manual.joomla.org for this PR? If so: Could you make a PR there?

@Fedik
Copy link
Member Author

Fedik commented Aug 10, 2025

Yes, I just added it for both PRs joomla/Manual#493

@richard67
Copy link
Member

@Fedik Thanks. That helps me maybe also with doing it for my PR #45878 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants