#57557 closed enhancement (fixed)
Switch copy_dir() with move_dir() in WP_Upgrader::install_package
Reported by: | afragen | Owned by: | 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
Change History (30)
This ticket was mentioned in PR #3911 on WordPress/wordpress-develop by @afragen.
22 months ago
#1
This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.
21 months ago
#4
follow-ups:
↓ 5
↓ 6
↓ 10
@
21 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
@
21 months ago
Replying to peterwilsoncc:
Big +1! :)
#6
in reply to:
↑ 4
@
21 months ago
Replying to peterwilsoncc:
I absolutely agree.
#10
in reply to:
↑ 4
;
follow-up:
↓ 14
@
21 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.
21 months ago
#12
@
21 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 ;)
21 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
@
21 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 callsupdate_core()
.
Yes, this seems to be the case for both manual and automatic core updates, full and partial.
#15
follow-up:
↓ 17
@
21 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?
#17
in reply to:
↑ 15
@
21 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
@
21 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
@
21 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.
This ticket was mentioned in PR #4004 on WordPress/wordpress-develop by @afragen.
21 months ago
#21
Language pack updates are partial updates and must go to copy_dir()
.
Trac ticket: https://core.trac.wordpress.org/ticket/57557
#23
follow-up:
↓ 26
@
21 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:
- Go to Settings -> General
- Under Site Language, choose a new language to install, like Français, and save the settings.
- The page will reload and the translation will be installed in the background.
- Notice how the dropdown now says Français is installed.
- Now, select Italiano from the dropdown and save the settings again.
- The page will reload and the translation will be installed in the background.
- Notice how the dropdown now says Italiano is installed, but Français is not.
To reproduce with WP-CLI
npm run env:start
npm run env:cli "wp language core install de_CH de_DE es_ES fr_FR it_IT"
- 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.
- 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. - Likewise, if I run
npm run env:cli "wp language core install de_CH"
, it_IT is wiped and only de_CH remains.
@SergeyBiryukov commented on PR #4004:
21 months ago
#25
Thanks for the PR! Merged in r55229.
#26
in reply to:
↑ 23
@
21 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
@
21 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.
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