Opened 3 months ago
Closed 4 weeks ago
#32529 closed enhancement (fixed)
wp-admin/includes files should not contain hooks
Reported by: | wonderboymusic | Owned by: | wonderboymusic |
---|---|---|---|
Milestone: | 4.3 | Priority: | normal |
Severity: | normal | Version: | 4.3 |
Component: | Administration | Keywords: | has-patch |
Focuses: | Cc: |
Description
Following up on #30947
There are random actions and filters littered among files like misc.php.
These files contain a bunch of functions that won't work outside of admin context and are typically only loaded in files that have already loaded the admin bootstrap.
These hooks should move to wp-admin/includes/admin-filters.php and wp-admin/includes/ms-admin-filters.php which can load where appropriate in wp-admin/includes/admin.php`.
A lot of these hooks (old media.php) are essentially deprecated.
Attachments (2)
Change History (14)
comment:1 @wonderboymusic — 3 months ago
comment:2 @DrewAPicture — 3 months ago
In 32671:
comment:3 @johnjamesjacoby — 3 months ago
A few things I'd like to see here:
- Consolidate actions vs. filters
- Column align parameters to make them easier to identify
- Looks like two network_admin_notices need to be moved to ms-default-filters.php
@johnjamesjacoby — 3 months ago
comment:4 @johnjamesjacoby — 3 months ago
32529.02.patch suggests the following:
- Alphabetize hooks by $tag
- One empty line between hook types
- Two empty lines between hook groups
- Column align a few lines for improved readability
Patch also moves network_admin_notices hooks to ms-default-filters.php.
I have not tested actual hook priorities; this patch is largely cosmetic.
comment:5 @wonderboymusic — 3 months ago
- Owner set to wonderboymusic
- Status changed from new to assigned
comment:6 @wonderboymusic — 3 months ago
- Resolution set to fixed
- Status changed from assigned to closed
In 32865:
comment:7 @azaozz — 5 weeks ago
- Resolution fixed deleted
- Status changed from closed to reopened
This sounds good but broke outputting the Add Media button above the editor in non wp-admin context, see #33257.
Looking through the files from where the actions and filters were moved, several more are sometimes included in non wp-admin context. Some plugins may also include them outside wp-admin.
In that terms seems we should revert this to maintain back-compat.
@wonderboymusic — 5 weeks ago
comment:8 @wonderboymusic — 5 weeks ago
Since the Editor class is an edge case where we load an admin file potentially on the front end, wouldn't 32529.diff work?
comment:9 @azaozz — 5 weeks ago
Yeah, that works. Don't even need to include ms-admin-filters.php.
Even simpler:
if ( ! function_exists( 'media_buttons' ) ) { include( ABSPATH . 'wp-admin/includes/media.php' ); add_action( 'media_buttons', 'media_buttons' ); }
The back-compat concern is mostly about plugins that include one of these files outside wp-admin. Also things like: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/update.php#L198.
comment:10 @slackbot — 4 weeks ago
This ticket was mentioned in Slack in #core by ocean90. View the logs.
comment:11 @azaozz — 4 weeks ago
Thinking that we can close this.
- https://core.trac.wordpress.org/attachment/ticket/33257/33257.diff fixes core and the known plugins use cases.
- Post on make/core and a mention in the email to plugin authors.
comment:12 @obenland — 4 weeks ago
- Resolution set to fixed
- Status changed from reopened to closed
In 32653: