WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#30947 closed enhancement (fixed)

wp-includes files should not contain hooks

Reported by: wonderboymusic Owned by: SergeyBiryukov
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.2
Component: General Keywords: has-patch 2nd-opinion 4.2-beta
Focuses: Cc:

Description

It is general good practice to have 2 types of PHP files: those that declare symbols, and those that cause side effects (vars, functions calls, class invocations, includes, etc).

There are some random add_action() and add_filter() calls littered around some files in wp-includes/. These should be moved to wp-includes/default-filters.php with the rest of the registered hooks. It seems like this was the best practice for awhile and then we randomly stopped. This file loads way before any of the includes, so the hooks will be registered for any request that loads WordPress, even SHORTINIT - a lot of the hooks registered won't run anyways (that's already the case).

I made sure the hooks are in the same order - corresponding to the order their files load.

Almost all static analysis tools + hackificator (see Hack) complain about this. Also a requirement of PSR-2, which I know we don't care about, but doesn't hurt.

Attachments (2)

30947.diff (15.3 KB) - added by wonderboymusic 6 years ago.
30947.2.diff (3.0 KB) - added by SergeyBiryukov 6 years ago.

Download all attachments as: .zip

Change History (20)

@wonderboymusic
6 years ago

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


6 years ago

#2 @rmccue
6 years ago

+1, totally agree with this. I've had times where I'm loading in files (typically loading parts of wp-includes from a page that doesn't automatically include them) and the code in them makes it impossible to use.

I'd suggest the same for wp-admin/includes/ too if we can.

#3 @wonderboymusic
6 years ago

In 31168:

There are some random add_action() and add_filter() calls littered around some files in wp-includes/. These should be moved to wp-includes/default-filters.php with the rest of the registered hooks. It seems like this was the best practice for awhile and then we randomly stopped. This file loads way before any of the includes, so the hooks will be registered for any request that loads WordPress, even SHORTINIT - a lot of the hooks registered won't run anyways (that's already the case).

See #30947.

#4 @wonderboymusic
6 years ago

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

wp-admin/includes/ is way trickier - since the add_*() calls are really only meant to run when the "template part" is loaded, which is not universal for the whole admin.

Let's mark this as done and maybe spin up a new ticket if we want to clean up the admin calls.

#5 @bobbingwide
6 years ago

  • Keywords 2nd-opinion added
  • Resolution fixed deleted
  • Status changed from closed to reopened

-1.

I think there may be a problem with the changes to update.php
since the code no longer takes into account this test that preceded the logic to add the actions.

if ( ( ! is_main_site() && ! is_network_admin() ) || ( defined( 'DOING_AJAX' ) && DOING_AJAX ) ) {
	return;
}

Question: What happens during AJAX processing for 'init'... will wp_schedule_update_checks() get invoked?

Answer: Yes, wp_schedule_update_checks will be run.
For instance. When you're editing a post and a heartbeat request is sent then this function is invoked.

Question: Will it work properly on multisite?
Answer: No.

Also... what's left at the bottom of the file is now rather unnecessary!

This is an unintended functional change.

I believe you'll need to revert the change to /wp-includes/update.php


#6 @SergeyBiryukov
6 years ago

30947.2.diff moves the reversed condition to default-filters.php.

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


6 years ago

#8 @DrewAPicture
6 years ago

  • Keywords 4.2-beta added

This can ride to Beta 1, see [31168]. @wonderboymusic: Anything left here?

#9 @ocean90
6 years ago

30947.2.diff needs to be committed.

#10 @SergeyBiryukov
6 years ago

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

In 31701:

Don't run update checks for AJAX requests after [31168].

fixes #30947.

#11 @rachelbaker
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

On an multisite install after [31701] I see this:

Notice: Trying to get property of non-object in /srv/www/wp/wp-includes/functions.php on line 3834

the wp-includes/default-filters.php is loaded before the $current_site global is set.

Last edited 6 years ago by rachelbaker (previous) (diff)

#12 @rachelbaker
6 years ago

#31587 was marked as a duplicate.

#13 @SergeyBiryukov
6 years ago

I guess we should revert the part of [31168] that [31701] was trying to fix.

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


6 years ago

#15 @SergeyBiryukov
6 years ago

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

In 31708:

Revert the part of [31168] that [31701] was trying to fix.

default-filters.php is loaded before the $current_site global is set, so is_main_site() cannot be used there.

fixes #30947.

#16 @nacin
5 years ago

default-filters.php should definitely remain free of pretty much anything. Good call with [31708].

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


5 years ago

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.