WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years 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 6 years ago.
32529.diff (766 bytes) - added by wonderboymusic 6 years ago.

Download all attachments as: .zip

Change History (14)

#1 @wonderboymusic
6 years 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.

#2 @DrewAPicture
6 years ago

In 32671:

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

See #32529.

#3 @johnjamesjacoby
6 years 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

#4 @johnjamesjacoby
6 years 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.

#5 @wonderboymusic
6 years ago

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

#6 @wonderboymusic
6 years ago

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

In 32865:

Cleanup (ms-)?admin-filters.php

Props johnjamesjacoby.
Fixes #32529.

#7 @azaozz
6 years 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
6 years ago

#8 @wonderboymusic
6 years 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?

#9 @azaozz
6 years 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 6 years ago by azaozz (previous) (diff)

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


6 years ago

#11 @azaozz
6 years ago

Thinking that we can close this.

#12 @obenland
6 years ago

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