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

PHPStan - Ensure Docblock presence for wpm_apply_filters_typed() #7119

Open
Miraeld opened this issue Nov 19, 2024 · 5 comments · May be fixed by #7132
Open

PHPStan - Ensure Docblock presence for wpm_apply_filters_typed() #7119

Miraeld opened this issue Nov 19, 2024 · 5 comments · May be fixed by #7132
Labels
type: enhancement Improvements that slightly enhance existing functionality and are fast to implement

Comments

@Miraeld
Copy link
Contributor

Miraeld commented Nov 19, 2024 •
edited
Loading

Add a PHPStan rule to ensure a docblock is present before the usage of wpm_apply_filters_typed(). The rule should also verify that the types declared in the docblock match the types of parameters passed to the filter.

Justification:
Code Quality: Ensures that all usages of wpm_apply_filters_typed() are well-documented, improving code readability and maintainability.
Type Safety: Verifies that the declared types in the docblock align with the actual parameters, reducing the risk of type-related errors.

Making sure that the following example is the template followed:

/**
 * Filter the rocket cache options.
 *
 * @param array $options Cache options.
 * @return array Filtered cache options.
 */
$options = wpm_apply_filters_typed( 'array', 'rocket_cache_options', $options );
@Miraeld Miraeld added the type: enhancement Improvements that slightly enhance existing functionality and are fast to implement label Nov 19, 2024
@remyperona
Copy link
Contributor

Draft PR wp-media/phpstan-apply-filters-typed#2

@Khadreal
Copy link
Contributor

Khadreal commented Nov 20, 2024 •
edited
Loading

Can I take over this @remyperona ? I can see the draft PR you created

@Miraeld
Copy link
Contributor Author

Miraeld commented Nov 21, 2024

⚠ @Khadreal
The draft PR is based on another repo, we need to bring back this rule within WP Rocket repo.

@Khadreal Khadreal linked a pull request Nov 22, 2024 that will close this issue
Open
7 tasks
@Khadreal Khadreal self-assigned this Nov 22, 2024
Khadreal added a commit that referenced this issue Nov 22, 2024
@Khadreal
Add constructor :closes: #7119
a7bfc41
@Miraeld Miraeld self-assigned this Nov 26, 2024
@MathieuLamiot
Copy link
Contributor

@Miraeld Can you update this issue to Blocked with a reference to the blocking issue (you mentioned a dedicated PR for tests?) Thanks 🙏

@Miraeld Miraeld mentioned this issue Nov 29, 2024
Open
9 tasks
@Miraeld
Copy link
Contributor Author

Miraeld commented Nov 29, 2024

Yes I was writing it,
It's been blocked because I'm not finding a solution with the unit tests.

To make it short and give some context. This new rule is checking the docblocks prior a wpm_apply_filters_typed call, making sure that there is a docblock and that types are corresponding as the one used in the function itself.

From what I'm seeing, the rules works, however, while adding unit tests, it doesn't take into account docblocks, it's ignoring them and says it doesn't detects docblocks.

I've created a sub-issue #7150 to tackle these tests. I've also created a PR for it, which could be use as a base. I cannot find a solution for it, and was waiting on @CrochetFeve0251 but he has been busy lately.

@Miraeld Miraeld removed their assignment Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Closes #7119 wp-media/wp-rocket
4 participants
@remyperona @Khadreal @Miraeld @MathieuLamiot