Miraeld commented 1 week ago

Description

Fixes #7120 Nothing impacts users.

Type of change

[x] Enhancement (non-breaking change which improves an existing functionality).

Detailed scenario

N/a

Technical description

Documentation

This pull request includes several changes to enhance the PHPStan configuration and introduce a custom rule to discourage the use of apply_filters in favor of a typed alternative. The key changes include adding a new script to generate a PHPStan baseline, updating the PHPStan configuration file to include the baseline and new paths, and implementing a custom PHPStan rule.

Enhancements to PHPStan configuration:

composer.json: Added a new script run-stan-reset-baseline to generate a PHPStan baseline.
phpstan.neon.dist: Included the phpstan-baseline.neon file and added new paths to the paths parameter. Also, set reportUnmatchedIgnoredErrors to false. [1] [2]

Introduction of a custom PHPStan rule:

phpstan.neon.dist: Added a new custom rule WP_Rocket\Tests\phpstan\Rules\DiscourageApplyFilters to the rules parameter.
tests/phpstan/Rules/DiscourageApplyFilters.php: Implemented the DiscourageApplyFilters class to discourage the use of apply_filters and suggest using wpm_apply_filters_typed instead.

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

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

Code style

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

MathieuLamiot commented 1 week ago

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 commented 1 week ago

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

MathieuLamiot commented 1 week ago

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

codacy-production[bot] commented 1 week ago

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for eda20949b04ea5756cff630dadd67c64ab81bfd9[^1]	:white_check_mark: ∅ (target: 50.00%)

Coverage variation details

| | Coverable lines | Covered lines | Coverage | | ------------- | ------------- | ------------- | ------------- | | Common ancestor commit (eda20949b04ea5756cff630dadd67c64ab81bfd9) | Report Missing | Report Missing | Report Missing | | | Head commit (0f29dc4ac59fc46f43c662fa8382782ea10de645) | 38228 | 16731 | 43.77% | **Coverage variation** is the difference between the coverage for the head and common ancestor commits of the pull request branch: ` - `

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: `/ * 100%`

See your quality gate settings Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more} [^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 commented 1 week ago

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

Miraeld commented 1 week ago

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.

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 ?

wp-media / wp-rocket

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

Description

Type of change

Detailed scenario

Technical description

Documentation

New dependencies

Risks

Mandatory Checklist

Code validation

Code style

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings Change summary preferences

Why We’re Using the PHPStan Baseline

What is the Baseline?

Updating the Baseline