WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 4 days ago

#36618 closed enhancement (fixed)

Move WP_Upgrader and WP_Upgrader_Skin subclasses into separate files

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-plugin-upgrader.php
wp-admin/includes/class-theme-upgrader.php
wp-admin/includes/class-language-pack-upgrader.php
wp-admin/includes/class-core-upgrader.php
wp-admin/includes/class-file-upload-upgrader.php
wp-admin/includes/class-wp-automatic-updater.php
wp-admin/includes/class-wp-upgrader-skins.php // Only for back-compat, requires files below.
wp-admin/includes/class-automatic-upgrader-skin.php
wp-admin/includes/class-wp-upgrader-skin.php
wp-admin/includes/class-bulk-upgrader-skin.php
wp-admin/includes/class-bulk-plugin-upgrader-skin.php
wp-admin/includes/class-bulk-theme-upgrader-skin.php
wp-admin/includes/class-language-pack-upgrader-skin.php
wp-admin/includes/class-plugin-upgrader-skin.php
wp-admin/includes/class-theme-upgrader-skin.php
wp-admin/includes/class-plugin-installer-skin.php
wp-admin/includes/class-theme-installer-skin.php

Thoughts?

Change History (26)

#1 follow-up: @rmccue
3 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
3 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
3 years ago

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

#4 @ocean90
3 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
3 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.


3 years ago

#7 @voldemortensen
3 years ago

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

#8 @jrf
3 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
3 years ago

  • Description modified (diff)

#10 follow-up: @ocean90
3 years ago

  • Keywords 2nd-opinion removed

Let's do this in stages. Each stage is one commit.

1) Create wp-admin/includes/class-automatic-upgrader-skin.php, wp-admin/includes/class-bulk-plugin-upgrader-skin.php, wp-admin/includes/class-bulk-theme-upgrader-skin.php, wp-admin/includes/class-bulk-upgrader-skin.php, wp-admin/includes/class-language-pack-upgrader-skin.php, wp-admin/includes/class-plugin-installer-skin.php, wp-admin/includes/class-plugin-upgrader-skin.php, wp-admin/includes/class-theme-installer-skin.php, wp-admin/includes/class-theme-upgrader-skin.php, and wp-admin/includes/class-wp-upgrader-skin.php as a copy of wp-admin/includes/class-wp-upgrader-skins.php.
2) Update file headers and DocBlocks for each new file.
3) Change wp-admin/includes/class-wp-upgrader-skins.php to require_once all the new files and update file header.

4) Create wp-admin/includes/class-plugin-upgrader.php, wp-admin/includes/class-theme-upgrader.php, wp-admin/includes/class-language-pack-upgrader.php, wp-admin/includes/class-core-upgrader.php, wp-admin/includes/class-file-upload-upgrader.php, and wp-admin/includes/class-wp-automatic-updater.php as a copy of wp-admin/includes/class-wp-upgrader.php.
5) Update file headers and DocBlocks for each new file.
6) Change wp-admin/includes/class-wp-upgrader.php to only include WP_Upgrader and add require_once for all the new files (upgrader and skins). Update file header.

7) If necessary, update file paths for hook docs.
8) Check references to include/require wp-admin/includes/class-wp-upgrader.php and if they can be changed to include/require only necessary files. File_Upload_Upgrader might to be one of those.

#11 @ocean90
3 years ago

In 37406:

Upgrader: Copy WP_Upgrader_Skin and its subclasses into one file per class.

Part 1/8.
See #36618.

#12 @ocean90
3 years ago

In 37407:

Upgrader: Update file headers and class DocBlocks for new files added in [37406].

Part 2/8.
See #36618.

#13 @ocean90
3 years ago

In 37408:

Upgrader: Update wp-admin/includes/class-wp-upgrader-skins.php to require_once the new files added in [37406].

Part 3/8.
See #36618.

#14 @ocean90
3 years ago

In 37409:

Upgrader: Copy WP_Upgrader subclasses into one file per class.

Part 4/8.
See #36618.

#15 @ocean90
3 years ago

In 37410:

Upgrader: Update file headers for new files added in [37409].

Part 5/8.
See #36618.

#16 @ocean90
3 years ago

In 37411:

Upgrader: Update wp-admin/includes/class-wp-upgrader.php to require_once the new files added in [37406] and [37409].

Part 6/8.
See #36618.

#17 @ocean90
3 years ago

In 37412:

Upgrader: After [37409] move the hook docs for upgrader_process_complete to WP_Upgrader::run().

Add changelog entry for [23912].

Part 7/8.
See #36618.

#18 in reply to: ↑ 10 @ocean90
3 years ago

Replying to ocean90:

8) Check references to include/require wp-admin/includes/class-wp-upgrader.php and if they can be changed to include/require only necessary files. File_Upload_Upgrader might to be one of those.

File_Upload_Upgrader is only used in /wp-admin/update.php which has an include_once for class-wp-upgrader.php on top. WP_Automatic_Updater is another class which is not a subclass of WP_Upgrader but it's using skins and other upgraders.
For now, we have to continue loading wp-admin/includes/class-wp-upgrader.php until we have an autoloader, see #36335.

#19 @swissspidy
3 years ago

#36804 was marked as a duplicate.

#20 follow-up: @swissspidy
3 years ago

@ocean90 Attention! [37411] broke update functionality in core.

There's a $package = ''; line in WP_Upgrader::download_package(), probably a leftover from debugging.

#21 @ocean90
3 years ago

In 37413:

Upgrader: Remove debug cruft.

See #36618.

#22 in reply to: ↑ 20 @ocean90
3 years ago

Replying to swissspidy:

@ocean90 Attention! [37411] broke update functionality in core.

There's a $package = ''; line in WP_Upgrader::download_package(), probably a leftover from debugging.

Thanks!

#23 @ocean90
3 years ago

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

#24 @ocean90
3 years ago

In 37432:

Upgrader: Add changelog entries for when the classes were moved to its own file.

See #36618.

#25 @DrewAPicture
3 years ago

In 38023:

Docs: Cross-reference parent classes in DocBlocks for upgrader classes moved to their own files in 4.6

See #36618. See #37318.

This ticket was mentioned in Slack in #core-committers by pento. View the logs.


4 days ago

Note: See TracTickets for help on using tickets.