Correct phpdoc example for Internal Pointers

[daleharrison]

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.

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

[daleharrison]

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?

[garrett-eclipse]

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

2. 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

[Luciano Croce]

@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... ;)

[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

[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.

[garrett-eclipse]

Awesome @daleharrison that's great news, thanks for the confirmation it wraps things up with a nice little bow ;)

[desrosj]

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.

[garrett-eclipse]

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.

[Luciano Croce]

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!

[Luciano Croce]

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!

[garrett-eclipse]

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

[Luciano Croce]

Sorry Brother!

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

[desrosj]

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?

[desrosj]

Tracked the introduction of this example to [19388].

[garrett-eclipse]

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.

[garrett-eclipse]

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.

[jrf]

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.

[garrett-eclipse]

Updated to follow convention

[garrett-eclipse]

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.

[jrf]

@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]

Refresh to reduce line-length of docblock code

[garrett-eclipse]

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.

[jrf]

👍🏻

[desrosj]

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.

[jrf]

@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]

Corrected Docblock example to avoid Closures

[garrett-eclipse]

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.

[jrf]

@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]

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

[garrett-eclipse]

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

[jrf]

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

[desrosj]

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

[desrosj]

In 46381:

Docs: Fix code example for removing internal pointers.

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