Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make (Local) Core Upgrades Work Again #1185

Merged
merged 2 commits into from Apr 1, 2019

Conversation

@mjangda
Copy link
Member

mjangda commented Mar 14, 2019

Pretty annoying that this this didn't work. If we're outside the VIP environment and in installing mode, just use the direct method for WP_Filesystem rather than blocking it.

Previously:

$ wp core upgrade
Updating to version 5.1.1 (en_US)...
Downloading update from https://downloads.wordpress.org/release/wordpress-5.1.1-no-content.zip...
Unpacking the update...
Warning: The `/vagrant/www/wordpress-default/public_html/wp-content/upgrade/wp_5c8aa1b7b5437` file cannot be managed by the `Automattic\VIP\Files\WP_Filesystem_VIP` class. Writes are only allowed for the `/uploads` and `/tmp` directories and reads can be performed everywhere. in /vagrant/www/wordpress-default/public_html/wp-content/mu-plugins/files/class-wp-filesystem-vip.php on line 65
Warning: The `/vagrant/www/wordpress-default/public_html/wp-content/upgrade/wp_5c8aa1b7b5437` file cannot be managed by the `Automattic\VIP\Files\WP_Filesystem_VIP` class. Writes are only allowed for the `/uploads` and `/tmp` directories and reads can be performed everywhere. in /vagrant/www/wordpress-default/public_html/wp-content/mu-plugins/files/class-wp-filesystem-vip.php on line 65
Error: Could not create directory.

Now:

$ wp core upgrade
Updating to version 5.1.1 (en_US)...
Using cached file '/home/vagrant/.wp-cli/cache/core/wordpress-5.1.1-no-content-en_US.zip'...
Unpacking the update...
Cleaning up files...
No files found that need cleaning up.
Success: WordPress updated successfully.

Fixes #1054

Checklist

Please make sure the items below have been covered before requesting a review:

  • This change works and has been tested locally (or has an appropriate fallback).
  • n/a This change works and has been tested on a Go sandbox.

Steps to Test

  1. Check out PR.
  2. Make sure your install is on an older version
  3. Run wp core update
  4. Verify it works.
  5. Downgrade back to an old version.
  6. Upgrade from wp-admin
  7. Verify it works.
  8. Make sure other Filesystem interaction works:
$ wp shell
$creds = request_filesystem_credentials( site_url() )
WP_Filesystem( $creds )
global $wp_filesystem

# this will not work (will throw a warning and return false)
$wp_filesystem->put_contents( 'file.txt', 'yay' )

# these will work
$wp_filesystem->put_contents( get_temp_dir() . 'file.txt', 'yay' )
$wp_filesystem->get_contents( get_temp_dir() . 'file.txt' )
$wp_filesystem->put_contents( wp_get_upload_dir()['basedir'] . '/file.txt', 'yay' )
$wp_filesystem->get_contents( wp_get_upload_dir()['basedir'] . '/file.txt' )```
mjangda added 2 commits Mar 14, 2019
Show the spcific paths rather than hard-coding them.
@mjangda

This comment has been minimized.

Copy link
Member Author

mjangda commented Mar 14, 2019

/cc @cameronterry @paulschreiber @hrkhal -- would love if y'all can test this out.

@hrkhal

This comment has been minimized.

Copy link
Contributor

hrkhal commented Mar 15, 2019

@mjangda Cheers for this.

I've only tested on my local env, but two things to note so far:

  1. It's very slow to unpack the download via the cli, it was not slow when upgrading via wp-admin

  2. This $wp_filesystem->put_contents( get_temp_dir() . 'file.txt, 'yay' ) did not work, it returns false. Equally I've tried to read it (incase it was a false flag) with $wp_filesystem->get_contents( get_temp_dir() . 'file.txt) but that also returned false. No error/warning message was returned.

Everything else worked as expected when going through your testing steps.

@mjangda

This comment has been minimized.

Copy link
Member Author

mjangda commented Mar 15, 2019

Thanks for testing this out @hrkhal! My test instructions were missing a line, sorry. Should be:

$ wp shell
$creds = request_filesystem_credentials( site_url() )
WP_Filesystem( $creds )
global $wp_filesystem

# this will not work (will throw a warning and return false)
$wp_filesystem->put_contents( 'file.txt', 'yay' )

# these will work
$wp_filesystem->put_contents( get_temp_dir() . 'file.txt', 'yay' )
$wp_filesystem->get_contents( get_temp_dir() . 'file.txt' )
$wp_filesystem->put_contents( wp_get_upload_dir()['basedir'] . '/file.txt', 'yay' )
$wp_filesystem->get_contents( wp_get_upload_dir()['basedir'] . '/file.txt' )
@hrkhal

This comment has been minimized.

Copy link
Contributor

hrkhal commented Mar 18, 2019

@mjangda I've repeated the last test step with your updated instructions and everything worked as expected. Nice one.

@mjangda mjangda requested a review from hanifn Mar 19, 2019
@paulschreiber

This comment has been minimized.

Copy link
Contributor

paulschreiber commented Mar 19, 2019

wp core update does not work on master; it works with this branch.

@mjangda mjangda merged commit bd2db89 into master Apr 1, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@mjangda mjangda deleted the fix/local-core-upgrade branch Apr 1, 2019
@mjangda

This comment has been minimized.

Copy link
Member Author

mjangda commented Apr 1, 2019

r135502-deploy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.