Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 17 months ago

#54795 closed defect (bug) (fixed)

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

Reported by: dlh's profile dlh Owned by: audrasjb's profile audrasjb
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Bootstrap/Load Keywords: has-patch commit assigned-for-commit
Focuses: Cc:

Description

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?

Attachments (1)

54795.diff (3.7 KB) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (19)

#1 @hellofromTonya
3 years ago

  • Milestone changed from Awaiting Review to 6.0
  • Version trunk deleted

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.

#2 @haykuro
2 years ago

#55435 was marked as a duplicate.

#3 @noisysocks
2 years ago

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.

#4 @antonvlasenko
2 years ago

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

This ticket was mentioned in PR #2560 on WordPress/wordpress-develop by anton-vlasenko.


2 years ago
#5

  • Keywords has-patch added

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

#6 @antonvlasenko
2 years ago

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

Last edited 2 years ago by antonvlasenko (previous) (diff)

#7 @audrasjb
2 years ago

  • Keywords needs-refresh added
  • Owner set to audrasjb
  • Status changed from new to reviewing

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

azaozz commented on PR #2560:


2 years ago
#8

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.

anton-vlasenko commented on PR #2560:


2 years ago
#9

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.

#10 in reply to: ↑ 8 @antonvlasenko
2 years ago

  • Keywords needs-refresh removed

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

#11 @audrasjb
2 years ago

  • Keywords commit assigned-for-commit added

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

Let's go with this 👍

#12 @audrasjb
2 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

@SergeyBiryukov
2 years ago

#14 follow-up: @SergeyBiryukov
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#15 @SergeyBiryukov
2 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 53304:

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

Follow-up to [53266].

Fixes #54795.

#16 in reply to: ↑ 14 ; follow-up: @antonvlasenko
2 years ago

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.

#17 in reply to: ↑ 16 @SergeyBiryukov
2 years ago

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.

#18 @johnbillion
17 months ago

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.

Note: See TracTickets for help on using tickets.