#54795 closed defect (bug) (fixed)
Bootstrap: Move navigation post type hooks to `admin-filters.php`
Reported by: | dlh | Owned by: | 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:
- 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.) - Should the three admin-only functions instead be globally included, not admin-only?
Attachments (1)
Change History (19)
#3
@
3 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.
This ticket was mentioned in PR #2560 on WordPress/wordpress-develop by anton-vlasenko.
3 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
@
3 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
#7
@
3 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?
3 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:
3 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
@
3 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
@
3 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 👍
3 years ago
#13
Committed in https://core.trac.wordpress.org/changeset/53266
#14
follow-up:
↓ 16
@
3 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.
#16
in reply to:
↑ 14
;
follow-up:
↓ 17
@
3 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
@
3 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
@
22 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.
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: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.