Make WordPress Core

Opened 15 months ago

Closed 15 months ago

Last modified 9 months ago

#57557 closed enhancement (fixed)

Switch copy_dir() with move_dir() in WP_Upgrader::install_package

Reported by: afragen's profile afragen Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.2 Priority: normal
Severity: normal Version: 6.2
Component: Upgrade/Install Keywords: has-patch
Focuses: performance Cc:

Description

While working extensively on #57375 in various discussions on that ticket and on Slack it was decided to move forward with PR3909.

PR3909 does not include the needed change to WP_Upgrader::install_package() to take advantage of its new code. This ticket and PR will accomplish that. The plan is to replace copy_dir() in WP_Upgrader::install_package() with move_dir().

This change was tested extensively during the development of #57375

Related: #57375, #57386

Change History (30)

This ticket was mentioned in PR #3911 on WordPress/wordpress-develop by @afragen.


15 months ago
#1

While working extensively on #57375 in various discussions on that ticket and on Slack it was decided to move forward with PR3909.

PR3909 does not include the needed change to WP_Upgrader::install_package() to take advantage of its new code. This ticket and PR will accomplish that. The plan is to replace copy_dir() in WP_Upgrader::install_package() with move_dir().

This change was tested extensively during the development of #57375

Related: #57375, #57386

Trac ticket: https://core.trac.wordpress.org/ticket/57557

@afragen commented on PR #3911:


15 months ago
#2

This PR will fail until PR3903 has been committed.

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


15 months ago

#4 follow-ups: @peterwilsoncc
15 months ago

Cross posting from my comment on #57375:

Discussing this with @azaozz, for 6.2 we'd like to limit the use of move_dir() to plugins and themes only. The Core updater ought to retain its current behavior.

This logic for this is simple: if the theme or plugin updater gets broken then it can be fixed by pushing out a minor update of WordPress. If the core updater gets broken then that's not so much of an option.

#5 in reply to: ↑ 4 @azaozz
15 months ago

Replying to peterwilsoncc:

Big +1! :)

#6 in reply to: ↑ 4 @afragen
15 months ago

Replying to peterwilsoncc:

I absolutely agree.

#7 follow-up: @mukesh27
15 months ago

  • Keywords changes-requested added

#8 in reply to: ↑ 7 @afragen
15 months ago

Replying to mukesh27:
What changes? I don’t see anything.

#9 @SergeyBiryukov
15 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#10 in reply to: ↑ 4 ; follow-up: @SergeyBiryukov
15 months ago

  • Keywords changes-requested removed

Replying to peterwilsoncc:

Discussing this with @azaozz, for 6.2 we'd like to limit the use of move_dir() to plugins and themes only. The Core updater ought to retain its current behavior.

It appears that the current PR does exactly that, as WP_Upgrader::install_package() only runs for plugin and theme updates. Core updates use Core_Upgrader::upgrade() instead, which in turn calls update_core().

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


15 months ago

#12 @peterwilsoncc
15 months ago

Thanks folks, I was hoping that was the case but would sure feel silly if I didn't ask and it wasn't ;)

@afragen commented on PR #3911:


15 months ago
#13

Tests are failing and will fail as move_dir() is not defined in core yet and it's not defined in this PR.

#14 in reply to: ↑ 10 @azaozz
15 months ago

Replying to SergeyBiryukov:

WP_Upgrader::install_package() only runs for plugin and theme updates.

Right, called through $this->run() from Plugin_Upgrader and Theme_Upgrader. The same is also used for language packs, see Language_Pack_Upgrader::bulk_upgrade(). As this runs automatically, wondering if this case should be excluded for now, just to be on the safe side.

Also wondering if it should be easier to make a "hotfix" plugin in case there are problems. Such plugin would help while preparing a .1 release (if needed).

Core updates use Core_Upgrader::upgrade() instead, which in turn calls update_core().

Yes, this seems to be the case for both manual and automatic core updates, full and partial.

#15 follow-up: @costdev
15 months ago

As this runs automatically, wondering if this case should be excluded for now, just to be on the safe side.

My only concern is that if we exclude language packs during the Beta/RC periods, we may have a difficult time getting a tester base for this. On the other hand, we can ask testers during Beta to test plugin, theme and language pack upgrades, and to report any issues they encounter with any of the upgrade types. That should expose any issues and give us the data we need.

Do we have any indication that language pack upgrades would encounter an issue? Despite happening automatically, we know they don't contain executable code, so a Fatal Error, etc shouldn't occur, OPcache invalidation won't be necessary for those, and the "move" portion of move_dir() is solid. Are there any other ways it could fail?

#16 @flixos90
15 months ago

  • Priority changed from high to normal

#17 in reply to: ↑ 15 @azaozz
15 months ago

Replying to costdev:

Do we have any indication that language pack upgrades would encounter an issue?

No. Just being overly-cautious I guess :)

On the other hand there are many weeks of testing so changing language pack updates to use copy_dir() can always be done if any problems are reported. So lets leave it as-is.

#18 @costdev
15 months ago

Sounds good!

If any issues are discovered with Language Pack upgrades during Beta and we need to restrict move_dir() to just Plugins/Themes while we investigate more, here's a snippet that will save us some time:

<?php

// src/wp-admin/includes/class-wp-upgrader.php#L597
if ( isset( $args['hook_extra']['plugin'] ) || isset( $args['hook_extra']['theme'] ) ) {
    $result = move_dir( $source, $remote_destination, true );
} else {
    $result = copy_dir( $source, $remote_destination );
}

Is this ticket ready to be added to the commit queue alongside the related tickets?

#19 @peterwilsoncc
15 months ago

@afragen, @costdev and I discussed when to use move or copy and it's a little more complicated than a simple switch from one to the other.

copy_dir needs to be used if WP_Upgrader::install_package() is called with either clear_working or clear_destination are false.

clear_working retains the source directory for the updated plugin in the upgrades folder. Core never sets the value to false but it can be altered via filters or when an extender calls the method directly.

clear_destination can be false in the case of partial updates and the files in the upgrade directory need to be appended to the existing directory. Again, this is not something Core does.

#20 @azaozz
15 months ago

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

In 55220:

Upgrade/Install: Use move_dir() instead of copy_dir() in WP_Upgrader::install_package() when possible. This would make the filesystem operations a lot faster in most cases, and potentially reduce failures.

Props: afragen, costdev, peterwilsoncc, pbiron, mukesh27, SergeyBiryukov, azaozz.
Fixes: #57557.

This ticket was mentioned in PR #4004 on WordPress/wordpress-develop by @afragen.


15 months ago
#21

Language pack updates are partial updates and must go to copy_dir().

Trac ticket: https://core.trac.wordpress.org/ticket/57557

#22 @afragen
15 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#23 follow-up: @swissspidy
15 months ago

It seems like this change broke translation installs/updates. It's no longer possible to have multiple languages installed.

To reproduce on WP trunk via wp-admin:

  1. Go to Settings -> General
  2. Under Site Language, choose a new language to install, like Français, and save the settings.
  3. The page will reload and the translation will be installed in the background.
  4. Notice how the dropdown now says Français is installed.
  5. Now, select Italiano from the dropdown and save the settings again.
  6. The page will reload and the translation will be installed in the background.
  7. Notice how the dropdown now says Italiano is installed, but Français is not.

To reproduce with WP-CLI

  1. npm run env:start
  2. npm run env:cli "wp language core install de_CH de_DE es_ES fr_FR it_IT"
  3. Notice how the WP-CLI output says all languages have been successfully installed. However, only the last language in the list (it_IT) is actually kept in the filesystem. The previous files have all been wiped.
  4. You can verify this with npm run env:cli "wp language core list --status=installed" or by going to Settings -> General and checking the language dropdown where it says only en_US and it_IT are installed.
  5. Likewise, if I run npm run env:cli "wp language core install de_CH", it_IT is wiped and only de_CH remains.

#24 @SergeyBiryukov
15 months ago

In 55229:

Upgrade/Install: Send language pack updates to copy_dir().

If the clear_working flag in WP_Upgrader::install_package() is false, the source should not be removed, so copy_dir() should be used instead.

Partial updates, like language packs, may want to retain the destination. If the destination exists or has contents, this may be a partial update, and the destination should not be removed, so copy_dir() should be used instead.

Follow-up to [55204], [55219], [55220], [55223], [55226].

Props afragen, costdev, swissspidy.
See #57557.

@SergeyBiryukov commented on PR #4004:


15 months ago
#25

Thanks for the PR! Merged in r55229.

#26 in reply to: ↑ 23 @SergeyBiryukov
15 months ago

Replying to swissspidy:

It seems like this change broke translation installs/updates. It's no longer possible to have multiple languages installed.

Thanks for the report! Does [55229] resolve this? I have refreshed the nightly build for easier testing.

#27 @swissspidy
15 months ago

Yes, looks like that resolves the issue, at least in manual tests. For some reason I also had to tweak my wp-env configuration due to file permissions, not sure if that's related though.

#28 @afragen
15 months ago

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

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


15 months ago

This ticket was mentioned in Slack in #core-media by joedolson. View the logs.


9 months ago

Note: See TracTickets for help on using tickets.