Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#45529 closed defect (bug) (fixed)

Correct phpdoc example for Internal Pointers

Reported by: daleharrison's profile daleharrison Owned by: desrosj's profile 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)

45529.diff (916 bytes) - added by garrett-eclipse 6 years ago.
45529.2.diff (973 bytes) - added by garrett-eclipse 5 years ago.
Updated to follow convention
45529.3.diff (1.0 KB) - added by garrett-eclipse 5 years ago.
Refresh to reduce line-length of docblock code
45529.4.diff (1.0 KB) - added by garrett-eclipse 5 years ago.
Corrected Docblock example to avoid Closures
45529.5.diff (1.0 KB) - added by garrett-eclipse 5 years ago.
Refreshed to include yourprefix_ and change to a more generic function name which is also shorter

Download all attachments as: .zip

Change History (42)

#1 follow-up: @swissspidy
6 years ago

  • Keywords close added

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' ) );
  }
};

#2 in reply to: ↑ 1 @daleharrison
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?

#3 @SergeyBiryukov
6 years ago

  • Component changed from Administration to Privacy
  • Focuses administration added

#4 @garrett-eclipse
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;

  1. Missing pointer_ prefix on pointer_wp496_privacy, all WP_Internal_Pointers are called with a pointer_ prefix on their add_action call.

Code Reference - https://github.com/WordPress/WordPress/blob/56c162fbc9867f923862f64f1b4570d885f1ff03/wp-admin/includes/class-wp-internal-pointers.php#L89

  1. Seems the priority on the admin_enqueue_scripts add_action needs to be 11. I've found for remove_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

Last edited 6 years ago by garrett-eclipse (previous) (diff)

#5 @Luciano Croce
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... ;)

#6 @Luciano Croce
6 years ago

  • Keywords needs-screenshots removed

#7 @swissspidy
6 years ago

  • Milestone Future Release deleted

#8 follow-up: @garrett-eclipse
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: @daleharrison
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 @garrett-eclipse
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 @desrosj
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 @garrett-eclipse
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 @Luciano Croce
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 @Luciano Croce
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 @garrett-eclipse
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 @Luciano Croce
6 years ago

Sorry Brother!

Yes I know but IMHO the problem is (also) a "fake user" called @luciano that do not exist!

#17 @desrosj
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 @desrosj
6 years ago

  • Version changed from 4.9.6 to 3.3

Tracked the introduction of this example to [19388].

#19 @garrett-eclipse
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 @garrett-eclipse
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: @jrf
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.

Last edited 5 years ago by jrf (previous) (diff)

@garrett-eclipse
5 years ago

Updated to follow convention

#25 in reply to: ↑ 24 @garrett-eclipse
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 @jrf
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 ;-)

@garrett-eclipse
5 years ago

Refresh to reduce line-length of docblock code

#27 @garrett-eclipse
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.

#28 @jrf
5 years ago

👍🏻

#29 @desrosj
5 years ago

  • Owner changed from garrett-eclipse to desrosj
  • Status changed from accepted to assigned

#30 @desrosj
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.

Last edited 5 years ago by desrosj (previous) (diff)

#31 @jrf
5 years ago

@desrosj Good point.

The thing is, in this case:

  1. Using a separate function would make the code sample even longer (though yes, it would comply with the coding standards better).
  2. 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.

@garrett-eclipse
5 years ago

Corrected Docblock example to avoid Closures

#32 @garrett-eclipse
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 @jrf
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.

@garrett-eclipse
5 years ago

Refreshed to include yourprefix_ and change to a more generic function name which is also shorter

#34 @garrett-eclipse
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

#35 @jrf
5 years ago

@garrett-eclipse Thanks for your patience in bearing with us. I'm totally fine with the current patch. 👍🏻

#36 @desrosj
5 years ago

Looks great to me too. Let's wrap this one up.

#37 @desrosj
5 years ago

  • Owner set to desrosj
  • Resolution set to fixed
  • Status changed from assigned to closed

In 46381:

Docs: Fix code example for removing internal pointers.

Props daleharrison, swissspidy, garrett-eclipse, luciano-croce, jrf, desrosj.
Fixes #45529.

Note: See TracTickets for help on using tickets.