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.

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

Ensure to not print the same hook too often #12

Closed
wants to merge 1 commit into from

Conversation

smileBeda
Copy link

Happens that sometimes a codebase does apply_filters or do_action the same hook more than once.
With the current template, you then get a DOC with each instance of that apply_filters or do_action repeated as many times as the codebase uses it.

For example, if you use a apply_filters( 'list_cats', $r['show_option_all'], null ) more than once (WP does that), your doc would result in a list of several list_cats, instead of just one, but you'd not want that.

With the proposed change, the filter or action, if already printed, will be skipped.
It might not be the best solution but avoids n-plicated occurrences of the same filter/action

NOTE
The template as such for me was not functional, I had to make changes to get it working, but those changes I will provide in another PR (if that's OK)

Happens that sometimes a codebase does `apply_filters` or `do_action` the _same_ hook more than once.
With the current template, you then get a DOC with each instance of that `apply_filters` or `do_action` repeated as many times as the codebase uses it.

For example, if you use a `apply_filters( 'list_cats', $r['show_option_all'], null )` more than once (WP does that), your doc would result in a list of several `list_cats`, instead of just one, but you'd not want that.

With the proposed change, the filter or action, if already printed, will be skipped.
It might not be the best solution but avoids n-plicated occurrences of the same filter/action

**NOTE**
The template as such for me was not functional, I had to make changes to get it working, but those changes I will provide in another PR (if that's OK)
@remcotolsma
Copy link
Member

We have also come across this, for us it was a reason to rewrite the code. We were often able to solve it by writing the code more "DRY", for example by moving the WordPress actions/filters to a function. In #14 we also added support the @ignore tag. I would rewrite the code or provide the duplicate calls with a @ignore tag.

https://github.com/WordPress/WordPress/search?q=list_cats

For the WordPress list_cats filter i don't think it's a very big issue, it's documented as:

list_cats

This filter is documented in wp-includes/category-template.php

Technically, the hook name might be the same, but the arguments might still be different. It may not even be that easy to correctly determine the differences. In that regard, the implementation in this PR may not cover all scenarios?

@smileBeda
Copy link
Author

You are probably right - and definitely with the @ignore tag for me this is solved anyway

I run into this duplicate issue only because I shimmed 2 functions from core to have them do what I need, and they use those filters.
So adding ignore to those in my shims is definitely the best solution for me in all cases

however - in terms of general “duplicated filter or action”, it shouldn’t be the arguments passed determining its documentation “repetitions”
Even wp core documents that filter only once. The developer will then have to know what he does with the filter in terms of returning the right values.
Otherwise we’d get each reused filter or action documented over and over again.

but for my project the ignore tag is good enough.

remcotolsma added a commit that referenced this pull request Jul 18, 2022
@remcotolsma
Copy link
Member

We added a test for this scenario in aa57bde#diff-b7800aeef93f4af1015b2cb95b7ac30ff063212b5b68c16f814cbe1a57aafb0b. It is difficult for this library to determine which filter/action documentation should or should not be included. With the @ignore tag, users can now determine this themselves. We will close this PR, if we get new insights we can always review / reconsider this.

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

Successfully merging this pull request may close these issues.

2 participants