Make WordPress Core

Opened 5 years ago

Last modified 17 months ago

#36618 closed enhancement

Move WP_Upgrader and WP_Upgrader_Skin subclasses into separate files — at Version 9

Reported by: ocean90 Owned by: ocean90
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: early
Focuses: Cc:

Description (last modified by ocean90)

Related: #34432, #36335

trunk/src/wp-admin/includes/class-wp-upgrader-skins.php includes WP_Upgrader_Skin, Plugin_Upgrader_Skin, Bulk_Upgrader_Skin, Plugin_Installer_Skin, Theme_Installer_Skin, Theme_Upgrader_Skin, Language_Pack_Upgrader_Skin, and Automatic_Upgrader_Skin.

trunk/src/wp-admin/includes/class-wp-upgrader.php includes WP_Upgrader, Plugin_Upgrader, Theme_Upgrader, Language_Pack_Upgrader, and Core_Upgrader.
The same file includes also File_Upload_Upgrader and WP_Automatic_Updater which are not a subclass of WP_Upgrader and the file includes class-wp-upgrader-skins.php.

Sadly, the naming of the classes isn't quite good because they don't have a unique prefix.

Based on the current naming schema we'd have the following new and old files:

wp-admin/includes/class-wp-upgrader.php // Includes WP_Upgrader and requires files below except fo class-wp-upgrader-skins.php.
wp-admin/includes/class-wp-upgrader-skins.php // Only for back-compat, requires files below.


Change History (9)

#1 follow-up: @rmccue
5 years ago

+100. The prefix doesn't matter too much, as they have a common suffix. (I have a autoloader written that already handles that. :) )

Splitting it would be pretty useful, although has the potential to break currently if someone is loading the file directly. That could be fixed by using an autoloader (#36335) to ensure the class is available regardless of what files are loaded manually.

#2 in reply to: ↑ 1 @ocean90
5 years ago

Replying to rmccue:

Splitting it would be pretty useful, although has the potential to break currently if someone is loading the file directly.

Can you elaborate on this? In my approach class-wp-upgrader.php will continue loading all the files.

#3 @rmccue
5 years ago

Just a general comment about splitting, not to this specific ticket. :)

#4 @ocean90
5 years ago

  • Keywords early added

Noting here that this needs some extensive testing on how it could affect a core upgrade.

@dd32 Any thoughts on this?

#5 @dd32
5 years ago

I have no objections, to either the main file just including everything else, or using a autoloader (probably in the future - Sorry there wasn't a common prefix!)

The only potential issue would be an incomplete failed update which then couldn't be completed from within wp-admin. As it is, the chances of a failed *major* update leaving WordPress in a state that WordPress can update itself again is rather slim.

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

5 years ago

#7 @voldemortensen
5 years ago

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

#8 @jrf
5 years ago

Related: https://core.trac.wordpress.org/ticket/34676, especially the second patch which essentially already addressed the splitting of the class to separate files: https://core.trac.wordpress.org/attachment/ticket/34676/34676.2.class-seperation-and-docs.diff

#9 @ocean90
5 years ago

  • Description modified (diff)
Note: See TracTickets for help on using tickets.