Opened 6 years ago
Closed 5 years ago
#45529 closed defect (bug) (fixed)
Correct phpdoc example for Internal Pointers
Reported by: | daleharrison | Owned by: | desrosj |
---|---|---|---|
Milestone: | 5.3 | Priority: | normal |
Severity: | normal | Version: | 3.3 |
Component: | Administration | Keywords: | has-patch commit |
Focuses: | docs, coding-standards | Cc: |
Description
I'm trying to disable the "Personal Data and Privacy" pointer for all users.
Unfortunately, it does not seem to be possible to do so via the remove_action() approach noted at the top of the WP_Internal_Pointers class.
This is the suggested approach:
<?php remove_action( 'admin_print_footer_scripts', array( 'WP_Internal_Pointers', 'pointer_wp390_widgets' ) );
…but I'm not getting the desired result when applied like so:
<?php remove_action( 'admin_print_footer_scripts', array( 'WP_Internal_Pointers', 'wp496_privacy' ) );
It is also worth noting that the developer of a plugin called Dismiss Privacy Nag seems to be bumping into the same issue: https://wordpress.org/plugins/dismiss-privacy-nag/#description.
Attachments (5)
Change History (42)
#2
in reply to:
↑ 1
@
6 years ago
Replying to swissspidy:
You should be able to do this:
<?php add_action( 'admin_enqueue_scripts', function() { remove_action( 'admin_print_footer_scripts', array( 'WP_Internal_Pointers', 'wp496_privacy' ) ); } );
Thanks, but unfortunately that's not working either. I tried it in a regular plugin, an mu-plugin, and my theme's functions.php file.
Note: I fixed the closing bracket of the add_action() call as quoted above.
Anything else I'm missing here or is this a legitimate bug?
#4
@
6 years ago
- Focuses docs added; administration removed
- Keywords has-patch dev-feedback added; close removed
- Milestone changed from Awaiting Review to Future Release
- Owner set to garrett-eclipse
- Status changed from new to assigned
Hi @daleharrison
Looking into this I found two changes the snippet you and @swissspidy provided;
- Missing
pointer_
prefix onpointer_wp496_privacy
, all WP_Internal_Pointers are called with apointer_
prefix on their add_action call.
Code Reference - https://github.com/WordPress/WordPress/blob/56c162fbc9867f923862f64f1b4570d885f1ff03/wp-admin/includes/class-wp-internal-pointers.php#L89
- Seems the priority on the
admin_enqueue_scripts
add_action needs to be 11. I've found forremove_action
's it's always best to increment your priority by 1.
Working code sample;
<?php add_action( 'admin_enqueue_scripts', function() { remove_action( 'admin_print_footer_scripts', array( 'WP_Internal_Pointers', 'pointer_wp496_privacy' ) ); }, 11 );
*This works from functions.php in your theme, tested on twentynineteen.
As well I passed this information to Luciano the dev of the plugin you mentioned as a PR with a working version of the above code.
PR - https://github.com/luciano-croce/dismiss-privacy-nag/pull/1
@swissspidy I added 45529.diff to update the inline phpdoc as the suggested code doesn't function as is, would you mind taking a look.
Thank you
#5
@
6 years ago
- Keywords needs-screenshots added
- Resolution set to worksforme
- Status changed from assigned to closed
@garrett-eclipse @swissspidy @daleharrison very good patch.
Tested locally (on my environment) work fine. :)
Work in functions.php of Themes:
- Twentynineteen
- Twentyseveteen
- Twentyeleven
Thanks!
P.S. I have updated both the code of my plugins Dismiss Privacy Nag and Dismiss Privacy Tools now... ;)
#8
follow-up:
↓ 14
@
6 years ago
- Milestone set to Future Release
- Resolution worksforme deleted
- Status changed from closed to reopened
Thanks @Luciano Croce
I'm glad that worked out for your plugins.
I'm re-opening this ticket as I added the 45529.diff patch to correct the insufficient phpdoc example as the existing example doesn't account for the need to have the remove_action called from a admin_enqueue_scripts
action with priority greater than 10.
@swissspidy would you mind looking at the updated example, I'm unsure if that's the best way to provide a multi-line code sample.
Appreciated
#9
follow-up:
↓ 13
@
6 years ago
Thanks @garrett-eclipse and @Luciano Croce, nice work. The code above is working perfectly for me in an mu-plugin, which is where I needed it to go.
#10
@
6 years ago
- Status changed from reopened to accepted
Awesome @daleharrison that's great news, thanks for the confirmation it wraps things up with a nice little bow ;)
#11
@
6 years ago
- Version changed from 4.9.8 to 4.9.6
I’d like to look into historically how long these pointer notices have remained in core, but I think that we can start exploring potentially removing this notice in either 5.1 or 5.2. It is meant as an upgrade notice to inform users of a new feature.
#12
@
6 years ago
- Summary changed from Can't Remove wp496_privacy From WP_Internal_Pointers to Correct phpdoc example for Internal Pointers
Thanks @desrosj, I agree we should be deprecating it in the near future. I've created #45999 for that purpose and to continue the discussion as well as to ping to open for others.
I've updated this ticket as the phpdoc example that proceeds the pointers function doesn't function without calling it appropriately and w/ priority. For this ticket my goal is simply to correct that example.
#13
in reply to:
↑ 9
@
6 years ago
Replying to daleharrison:
Thanks @garrett-eclipse and @Luciano Croce, nice work. The code above is working perfectly for me in an mu-plugin, which is where I needed it to go.
Remember that my username for "quote me" is luciano-croce @luciano-croce Thanks!
#14
in reply to:
↑ 8
@
6 years ago
Replying to garrett-eclipse:
Thanks @Luciano Croce
I'm glad that worked out for your plugins.
I'm re-opening this ticket as I added the 45529.diff patch to correct the insufficient phpdoc example as the existing example doesn't account for the need to have the remove_action called from a
admin_enqueue_scripts
action with priority greater than 10.
@swissspidy would you mind looking at the updated example, I'm unsure if that's the best way to provide a multi-line code sample.
Appreciated
Remember that my username for "quote me" is luciano-croce @luciano-croce Thanks!
#15
@
6 years ago
Hi @luciano-croce, yes seems your username on Trac is broken. When selecting you from the auto-suggestion and when adding a reply to you it uses @Luciano Croce
which breaks on the space.
See screen here for the autosuggestion issue - https://i.imgur.com/68lr2rw.png
Can you raise in #meta to have your username fixed on Trac as the system is using an invalid one and I doubt anyone will remember.
Thanks
#16
@
6 years ago
Sorry Brother!
Yes I know but IMHO the problem is (also) a "fake user" called @luciano that do not exist!
#17
@
6 years ago
- Component changed from Privacy to Administration
- Keywords needs-refresh added; dev-feedback removed
- Milestone changed from Future Release to 5.2
Since this is a documentation improvement, let's fix this in 5.2. Also, pointers are an Administration feature.
The updated example in 45529.diff uses an anonymous function, which is only supported in PHP 5.3+.
Instead of changing the example, can it just note the action hook and priority?
#18
@
6 years ago
- Version changed from 4.9.6 to 3.3
Tracked the introduction of this example to [19388].
#19
@
6 years ago
Thanks @desrosj 5.2 sounds good.
I'm not sure the best approach there as the example will also become outdated once the pointer in the example is deprecated. Do you know of any other instances where a code sample is provided in the phpdoc block?
Possibly the example should be moved into a codex page or somewhere we can then use a permalink to replace the code example with?
I did look at how we would note the action hook and priority but was hard to come up with something understandable as it's a double function and the remove action uses an array to define the WP_Internal_Pointers class. What I was left with on my attempt to write something was;
"Individual pointers can be disabled by wrapping an add_action for admin_enqueue_scripts
with a priority of 11 around a remove_action for admin_print_footer_scripts
to remove the pointer function name with point_
prefix from the WP_Internal_Pointers class"
*I tried to rewrite the function using the above and was hard pressed, I feel the example is worth 1000 words in this case as it clearly shows something that can be copy pasted.
Would love to get your thoughts and can refresh accordingly.
#20
@
6 years ago
- Keywords 2nd-opinion added
- Milestone changed from 5.2 to 5.3
Moving this off of the 5.2 milestone as it's no longer a priority seeing that r44787 is now in trunk which removes the privacy pointer leaving no active pointers.
Added 2nd-opinion to get feedback as to the correct approach to introducing a code block to phpdoc and if it should be a @link insteat.
This ticket was mentioned in Slack in #core by david.baumwald. View the logs.
5 years ago
This ticket was mentioned in Slack in #core by david.baumwald. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-coding-standards by garrett-eclipse. View the logs.
5 years ago
#24
follow-up:
↓ 25
@
5 years ago
Hi @garrett-eclipse I saw your question on Slack about how to do and/or whether to have code samples in the docblock.
Where appropriate, this is perfectly fine to use.
There's even a Docs coding standard covering it:
https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#description-formerly-long-description
Note: It is strongly recommended that the code sample complies with the PHP coding standards.
Looking at your patch, looks like all you need to change is:
- Add a blank (comment) line before the code sample.
- Change the number of spaces for the initial indentation of the code sample from 5 to 4.
Hope this helps.
#25
in reply to:
↑ 24
@
5 years ago
- Focuses coding-standards added
- Keywords commit added; needs-refresh 2nd-opinion removed
Thanks @jrf I greatly appreciate you popping over.
This is exactly what I was seeking here, and the fact it's already documented is the cherry on top.
I've updated the patch with your recommendations in 45529.2.diff, would you mind providing a final review.
#26
@
5 years ago
@garrett-eclipse New patch LGTM based on the standards.
Strictly speaking line 29 is a bit long and could be broken up to a multi-line function call, but for the purposes of this code sample, I'm totally fine with it as is.
Fingers crossed that when the docs get regenerated, it all works as expected ;-)
#27
@
5 years ago
Thanks @jrf and good point as always, I refreshed in 45529.3.diff to reduce line-length by making the remove_action
call multi-line.
#29
@
5 years ago
- Owner changed from garrett-eclipse to desrosj
- Status changed from accepted to assigned
#30
@
5 years ago
- Keywords 2nd-opinion added
- Owner desrosj deleted
While I am not opposed to 45529.3.diff, I did want to question the use of a closure on an action hook.
The latest updates to the coding standards established that:
Closures must not be passed as filter or action callbacks, as they cannot be removed by
remove_action()
/remove_filter()
Because of that, I am hesitant to introduce this as the recommended example here.
Edit: added link to the blog post updating the coding standards.
#31
@
5 years ago
@desrosj Good point.
The thing is, in this case:
- Using a separate function would make the code sample even longer (though yes, it would comply with the coding standards better).
- The closure is used to remove the action and, while it can not be unhooked, an
add_action()
at prio 12 could easily re-add the removed action.
So, I'm in two minds about this.
#32
@
5 years ago
- Keywords 2nd-opinion removed
Thanks @desrosj and @jrf for the feedback.
I've refreshed in 45529.4.diff to remove the enclosure.
After implementing I found we can allay your first point @jrf as the codeblock is actually less lines and less indenting than the Closure due to the fact it was unfurled to multi-line syntax. So as they say short and sweet ;)
@desrosj & @jrf can you have a final review and make sure I didn't miss anything in my changes.
#33
@
5 years ago
@garrett-eclipse Wow, that's a quick turn-around ;-)
Reviewed. Latest patch looks nearly perfect to me. The only thing I would suggest is to prefix the function.
Plugins and themes should always prefix everything they add to the global namespace to avoid naming collisions, so using yourprefix_remove_pointer_wp390_widgets
as the function name should probably convey that point sufficiently.
@
5 years ago
Refreshed to include yourprefix_
and change to a more generic function name which is also shorter
#34
@
5 years ago
Thanks @jrf here's another for you to review in 45529.5.diff.
I added the yourprefix_
prefix and found it becomes excessively long, looking at it further the general wrapper action can be more generic in name as any or multiple pointer removal calls can be included within it. As such I've gone with yourprefix_remove_pointers
.
All the best
You should be able to do this: