Bootstrap: Move navigation post type hooks to `admin-filters.php`

[dlh]

WordPress 5.9 adds some new actions and filters to default-filters.php into hooks that typically run only in the admin, such as admin_head and admin_footer-widgets.php.

I can see that there are plenty of other similar examples that predate 5.9, and, generally, it's not a big deal.

However, three of these new hooks go one step further by adding callbacks that are themselves only loaded in the admin:

add_action( 'use_block_editor_for_post_type', '_disable_block_editor_for_navigation_post_type', 10, 2 );
add_action( 'edit_form_after_title', '_disable_content_editor_for_navigation_post_type' );
add_action( 'edit_form_after_editor', '_enable_content_editor_for_navigation_post_type' );

There is precedent, too, for adding admin-only callbacks in default-filters.php, but the implications are more noticeable: On PHP 8+, a fatal error will occur if those hooks fire in a non-admin context and attempt to call an undefined function.

I hear you asking: Isn't somebody doing it wrong if those hooks run outside the admin? Probably. Did I, myself, manually apply the use_block_editor_for_post_type filter in a plugin? I sure did, and I'll fix the plugin :)

Still, I was on the fence about whether adding admin-only functions to default-filters.php is also not quite doing it right. It risks creating new warnings or fatal errors without serving an obvious (to me) purpose for core. [32653] described admin-filters.php as "[containing] functions that won't work outside of admin context." So, I thought I would open a ticket to ask:

1. Should the new default actions and filters in 5.9 that call admin-only functions be moved to admin-filters.php? (They seemed to work as expected there when I tested.)
2. Should the three admin-only functions instead be globally included, not admin-only?

[hellofromTonya]

Hello @dlh,

That's a great find and something that needs consideration for the reasons you raised.

A quick walkthrough of the wp-includes/default-filters.php file has these "admin" hooks:

• 5.8 in [51003] adds:

  add_action( 'admin_footer-post.php', 'wp_add_iframed_editor_assets_html' );
  add_action( 'admin_footer-post-new.php', 'wp_add_iframed_editor_assets_html' );
  

• 5.8 in [51388] adds:

  add_action( 'admin_head', 'wp_check_widget_editor_deps' );
  

• 5.9 in [52069] adds:

  add_action( 'admin_footer-widgets.php', 'wp_add_iframed_editor_assets_html' );
  

• 5.9 in [52160] adds:

  add_action( 'admin_footer-site-editor.php', 'wp_add_iframed_editor_assets_html' );
  

• 5.9 in [52145] adds:

  add_action( 'use_block_editor_for_post_type', '_disable_block_editor_for_navigation_post_type', 10, 2 );
  add_action( 'edit_form_after_title', '_disable_content_editor_for_navigation_post_type' );
  add_action( 'edit_form_after_editor', '_enable_content_editor_for_navigation_post_type' );
  

There are other "admin" hooks in the file too for Customizer, calendar widget cache, post submenus (hooked to admin_menu), etc.

I agree that an audit is needed to identify admin-only functions, i.e. callbacks that are not loaded into memory when not in the admin area.

Given that there are "admin" hooks in the file before 5.9, removing trunk as the Version and moving this ticket into 6.0.

[haykuro]

#55435 was marked as a duplicate.

[noisysocks]

Should the new default actions and filters in 5.9 that call admin-only functions be moved to admin-filters.php? (They seemed to work as expected there when I tested.)

This sounds right to me.

[antonvlasenko]

I've started to work on a patch for this issue.

This PR aims to move admin hooks to admin-filters.php.
These hooks are not supposed to be used from a non-admin context.

Trac ticket: https://core.trac.wordpress.org/ticket/54795

[antonvlasenko]

I've investigated hooks present in the default-filters.php file.
Out of more than 400 hooks listed in that file, only 9 hooks don't have available callbacks. So it means that these 9 callback functions should be called from the admin context. I've checked the source code, and I confirm that.
I've moved 6 hooks to the admin-filters.php file.
According to ​this comment in the default-filters.php file, the remaining 3 hooks could be used on the front-end as well so I didn't move them.
I'd appreciate a quick review and merge of ​my PR as WP 6.0 Beta 1 should be released tomorrow.

FYI: @noisysocks

[audrasjb]

@antonvlasenko the PR looks good to me, but there is a few conflict that would need to be solved. Are you available to resolve them in the current PR?

Also thinking it would be good to extend the file headers/docblocks on both files and clearly state the intended use: admin hooks should be in admin-filters.php, etc.

Also thinking it would be good to extend the file headers/docblocks on both files and clearly state the intended use: admin hooks should be in admin-filters.php, etc.

Fixed in b57b84b0bda03d0471c685af89bc18680ada41a0
As for adding a note to admin-filters.php, I'm not sure which wording to use.
The issue is that admin-specific filters have been added to default-filters.php and not vice versa.
If you have any suggestions, please feel free to modify this PR.
Thank you for the feedback, @azaozz.

[antonvlasenko]

@audrasjb
I've resolved the merge conflict and added a note to the default-filters.php file (per @azaozz 's suggestion).
Thank you.

[audrasjb]

Thanks! We should be good to go then.
(I tweaked a bit the note)

Let's go with this 👍

[audrasjb]

In 53266:

Bootstrap/load: Move administration related hooks to admin-filters.php.

This change moves some administration related hooks from default-filters.php to admin-filters.php. It also updates the default-filters.php docblock to indicate that contextualized hooks should be located in the most appropriate place.

Props dlh, hellofromTonya, antonvlasenko, audrasjb, azaozz.
Fixes #54795.

Committed in https://core.trac.wordpress.org/changeset/53266

[SergeyBiryukov]

I think there are a few more calls that can be safely moved for consistency, see 54795.diff​.

[SergeyBiryukov]

In 53304:

Bootstrap/Load: Move some more administration-related hooks to admin-filters.php.

Follow-up to [53266].

Fixes #54795.

[antonvlasenko]

Replying to SergeyBiryukov:

I think there are a few more calls that can be safely moved for consistency, see 54795.diff​.

In my pull request, I was more focused on hooks that definitely wouldn't work on the front end (because callback functions weren't available for them).
Unfortunately, due to the time limit, it was not possible for me to manually check all ~400 hooks in the default-filters.php file. I only did some automated checks.
Thank you for the extra effort on this task.

[SergeyBiryukov]

Replying to antonvlasenko:

In my pull request, I was more focused on hooks that definitely wouldn't work on the front end (because callback functions weren't available for them).

That was my understanding too, thanks for confirming!

Just noting that there are still a few admin_* hooks left in default-filters.php, but those seem to be closely related to other non-admin filters, so I think it's fine to leave them as is for now for better context. In [53304], I was only focused on those that don't have any close ties and are a better fit for admin-filters.php.

[johnbillion]

I think we need to be more careful with these changes in the future. [53304] looks fine on the surface but actually subtly changes the order in which actions are added. I just had to spend some time debugging a plugin of mine which broke in 6.0 (the breakage had gone unnoticed) because its admin_print_footer_scripts callback had switched from running after _wp_footer_scripts() to running before it. This is because admin-filters.php is called much later than default-filters.php.

Not a huge problem in my particular case, but let's bear it in mind for any similar changes in the future.