Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Closes #7120: PHPStan - Discourage apply_filters in Favor of wpm_apply_filters_typed() #7121

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Closes #7120: PHPStan - Discourage apply_filters in Favor of wpm_apply_filters_typed() #7121

wants to merge 3 commits into from
+1,517 −0

Conversation

Miraeld
Copy link
Contributor

@Miraeld Miraeld commented Nov 20, 2024

Description

The CI is failing, that's normal, we are still using apply_filters at the moment. It needs refactor

Fixes #7120
Nothing impacts users.

Type of change

  • Enhancement (non-breaking change which improves an existing functionality).

Detailed scenario

N/a

Technical description

Documentation

This will trigger an error with PHPStan if we are using apply_filters and recommend to use wpm_apply_filters_typed().
Example:

 ------ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
  Line   tests/Integration/inc/ThirdParty/Themes/Uncode/excludeDelayJs.php                                                                                                                                                          
 ------ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
  26     Usage of apply_filters() is discouraged. Use wpm_apply_filters_typed() instead.                                                                                                                                            
         💡 We've created a wpm_apply_filters library to help you type hint your filters. You can use it to type hint your filters and make your code more predictable. More info: https://github.com/wp-media/apply-filters-typed  
  69     Usage of apply_filters() is discouraged. Use wpm_apply_filters_typed() instead.                                                                                                                                            
         💡 We've created a wpm_apply_filters library to help you type hint your filters. You can use it to type hint your filters and make your code more predictable. More info: https://github.com/wp-media/apply-filters-typed  
 ------ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
  Line   tests/Integration/inc/ThirdParty/Themes/Uncode/excludeJs.php                                                                                                                                                               
 ------ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
  26     Usage of apply_filters() is discouraged. Use wpm_apply_filters_typed() instead.                                                                                                                                            
         💡 We've created a wpm_apply_filters library to help you type hint your filters. You can use it to type hint your filters and make your code more predictable. More info: https://github.com/wp-media/apply-filters-typed  
  67     Usage of apply_filters() is discouraged. Use wpm_apply_filters_typed() instead.                                                                                                                                            
         💡 We've created a wpm_apply_filters library to help you type hint your filters. You can use it to type hint your filters and make your code more predictable. More info: https://github.com/wp-media/apply-filters-typed  
 ------ ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

New dependencies

n/a

Risks

n/a

Mandatory Checklist

Code validation

  • I validated all the Acceptance Criteria. If possible, provide screenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I implemented built-in tests to cover the new/changed code.

Code style

  • I wrote a self-explanatory code about what it does.
  • I protected entry points against unexpected inputs.
  • I did not introduce unnecessary complexity.
  • Output messages (errors, notices, logs) are explicit enough for users to understand the issue and are actionnable.

@Miraeld Miraeld added the type: enhancement Improvements that slightly enhance existing functionality and are fast to implement label Nov 20, 2024
@Miraeld Miraeld self-assigned this Nov 20, 2024
@MathieuLamiot
Copy link
Contributor

Thanks @Miraeld !
To avoid breaking theme CI on the repo, could we apply this rule on the diff only for now? I think PHPCS offers this possibility.
We would benefit from the rule without having to update the whole codebase for now.

@Miraeld
Copy link
Contributor Author

Miraeld commented Nov 20, 2024

@MathieuLamiot Phpstan doesn't offer this possibility, per se the documentation.

@MathieuLamiot
Copy link
Contributor

@Miraeld what about this? https://phpstan.org/user-guide/baseline

@Miraeld Miraeld force-pushed the enhancement/7120-add-new-rule-phpstan branch from 0222279 to b6f887a Compare November 20, 2024 09:17
@codacy-production Codacy Production
Copy link

codacy-production bot commented Nov 20, 2024 •
edited
Loading

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for eda20941 ✅ ∅ (target: 50.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (eda2094) Report Missing Report Missing Report Missing
Head commit (0f29dc4) 38228 16731 43.77%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#7121) 0 0 ∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Footnotes

  1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

@Miraeld Miraeld force-pushed the enhancement/7120-add-new-rule-phpstan branch from 9608d2b to ecde571 Compare November 20, 2024 10:31
@Miraeld Miraeld requested review from a team and CrochetFeve0251 November 20, 2024 10:32
@Miraeld
Copy link
Contributor Author

Miraeld commented Nov 20, 2024

A baseline system has been introduced in this PR. A notion page will be created to explain more about it.

wordpressfan
wordpressfan requested changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@wordpressfan wordpressfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work with you locally?
It doesn't work with me at all, I tested it like that:
Add apply_filters( 'rocket_test', false ) to the code inside inc/Engine and run the command:

composer run run-stan-reset-baseline

but it shows that everything is OK.

image

Am I doing anything wrong here?

Also, do we need to handle the function apply_filters_ref_array also here?

@Miraeld
Copy link
Contributor Author

Miraeld commented Nov 22, 2024 •
edited
Loading

Does this work with you locally? It doesn't work with me at all, I tested it like that: Add apply_filters( 'rocket_test', false ) to the code inside inc/Engine and run the command:

composer run run-stan-reset-baseline

but it shows that everything is OK.

image

Am I doing anything wrong here?

Also, do we need to handle the function apply_filters_ref_array also here?

Yea it's normal you are using the wrong command.

Okay, let me explain quick here, and I'll explain deeper on Slack I think.

Why We’re Using the PHPStan Baseline

Introducing this new PHPStan rule across our codebase results in 557 errors because of how extensively we use apply_filters. However, the goal of this PR is not to refactor all these instances but to implement the rule itself.

To manage these existing issues without addressing them right now, we're leveraging PHPStan's baseline system.

What is the Baseline?

The baseline acts as a "to-do list" for PHPStan. It records known errors and tells PHPStan to "ignore these for now." This allows us to focus on new issues while keeping the existing ones acknowledged but not flagged during analysis.

Updating the Baseline

The Composer command I added (run-stan-reset-baseline) lets you update the baseline file with the latest list of known errors. This ensures PHPStan won't report the existing 557 issues during its checks.

This approach allows us to adopt the new rule without overwhelming ourselves with fixing historical issues immediately. Let me know if you need further elaboration!

About apply_filters_ref_array, I'm not sure, what @CrochetFeve0251 & @remyperona think about it ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wordpressfan wordpressfan wordpressfan requested changes

@CrochetFeve0251 CrochetFeve0251 Awaiting requested review from CrochetFeve0251

Assignees

@Miraeld Miraeld

Labels
type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PHPStan - Discourage apply_filters in Favor of wpm_apply_filters_typed()
3 participants
@Miraeld @MathieuLamiot @wordpressfan