WordPress.org

Make WordPress Core

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)

32529.02.patch (7.6 KB) - added by johnjamesjacoby 3 months ago.
32529.diff (766 bytes) - added by wonderboymusic 5 weeks ago.

Download all attachments as: .zip

Change History (14)

comment:1 @wonderboymusic3 months ago

In 32653:

In the style of #30947 and default-filters.php, add 2 new files to wp-admin/includes:
admin-filters.php
ms-admin-filters.php

There are random actions and filters littered among files like misc.php. These files contain functions that won't work outside of admin context and are typically only loaded in files that have already loaded the admin bootstrap.

See #32529.

comment:2 @DrewAPicture3 months ago

In 32671:

Fix inline documentation formatting in wp-admin/includes/admin-filters.php, introduced in [32653].

See #32529.

comment:3 @johnjamesjacoby3 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

@johnjamesjacoby3 months ago

comment:4 @johnjamesjacoby3 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 @wonderboymusic3 months ago

  • Owner set to wonderboymusic
  • Status changed from new to assigned

comment:6 @wonderboymusic3 months ago

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

In 32865:

Cleanup (ms-)?admin-filters.php

Props johnjamesjacoby.
Fixes #32529.

comment:7 @azaozz5 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.

@wonderboymusic5 weeks ago

comment:8 @wonderboymusic5 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 @azaozz5 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.

Last edited 5 weeks ago by azaozz (previous) (diff)

comment:10 @slackbot4 weeks ago

This ticket was mentioned in Slack in #core by ocean90. View the logs.

comment:11 @azaozz4 weeks ago

Thinking that we can close this.

comment:12 @obenland4 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.