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

Ensure plugin, theme, and WordPress updates succeed #1431

Conversation

SiobhyB
Copy link

@SiobhyB SiobhyB commented May 20, 2024

What is this PR doing?

This PR ensures plugin, theme, and WordPress updates always succeed.

What problem is it solving?

Issues related to updates can be reproduced in the Studio app, wp-now, and Playground CLI. Specifically, attempting to update outdated versions of WordPress, plugins, or themes results in an error when using any of the aforementioned tools.

Studio issues: https://github.com/Automattic/dotcom-forge/issues/6891, Automattic/studio#100

wp-now issue: WordPress/playground-tools#178

How is the problem addressed?

With this PR, we add old_node.parent = new_dir to the finally block in the rename handler to ensure the parent node is correctly updated, even if it's not correctly handled during the rename operation.

Although this shouldn't technically be necessary, as the parent is also set here as part of the try block, I believe some of the differences in file systems and types can contribute to inconsistency. To understand this, it's important to have the following context to this problem:

  • WordPress calls PHP's rename() as part of its installation and update processes here.
  • It was found that updates work when the rename() functionality was commented out or after adding $this->is_file( $source ) && to the conditional.
  • The errors related to updates can only be reproduced in environments using a VFS, and logs indicate that the VFS was not being correctly deleted for the source.

By explicitly updating the parent node in the finally block, we guarantee consistent filesystem state and proper handling of directories across different scenarios.

Testing Instructions

To test these changes, first use Playground CLI to spin up a local WordPress environment. In my case, I used an existing WordPress installation in my local Studio directory and the following command. /../Studio/my-wordpress-website should be updated to point to your specific local installation:

bun packages/playground/cli/src/cli.ts server --skipWordPressSetup --mount-before-install=$(pwd)/../Studio/my-wordpress-website:/wordpress

Update an outdated plugin

Tip

I encountered a persistent error when attempting to install the Jetpack plugin using Playground CLI. It was possible to reproduce this against trunk, as such I recommend using other plugins for testing.

  • Install an outdated version of a plugin via /wp-admin's Plugins section. One way to do this is to navigate to the Advanced View on a plugin's WordPress.org page and scroll to the bottom for a dropdown of older downloads. Example: Gutenberg's Advanced View page.
  • Refresh the Plugins section and choose the option to update alongside your newly installed plugin.
  • Verify it updates as expected without error. You should notice the version number alongside the plugin change to the latest.

Recording:

updates.mov

Update an outdated version of WordPress

  • Roll your local WordPress environment back to an older version.
  • Click any of the prompts to update to the latest version of WordPress. These will generally be found in the header or footer section of any admin page, or by navigating directly to DashboardUpdates in /wp-admin.
  • Verify that the update is successful and that your site is now using the latest available version of WordPress.

Recording:

wordpress-update.mov

General smoke testing

Ensure no regressions by testing general functionality related to installations. It should still be possible to install plugins and themes as before.

Added `old_node.parent = new_dir` to the `finally` block in the `rename` function to ensure that the directory structure and hash table remain consistent.
@SiobhyB SiobhyB changed the title fix: Ensure consistency during rename operations Ensure plugin, theme, and WordPress updates succeed May 20, 2024
@adamziel
Copy link
Collaborator

@SiobhyB great fix! One note I have – this PR updates autogenerated files and the changes will be lost on the first rebuild. This patch would better suit the code generator – Emscripten. This is the relevant section of code:

https://github.com/emscripten-core/emscripten/blob/2e3d7f108f0c278f1aab56bebac4497f7a28ec46/src/library_fs.js#L817-L821

@SiobhyB
Copy link
Author

SiobhyB commented May 20, 2024

@adamziel, thank you! Have Playground contributors had success in getting PRs approved in the Emscripten repository?

I'm a bit concerned this might not be accepted as old_node.parent = new_dir is technically already being called here as part of the try block. As I've set out in the PR's How is the problem addressed? section, I think the need to add it to the finally block may be specific to our use case. But there could also be a case for this being useful for others.

@SiobhyB
Copy link
Author

SiobhyB commented May 20, 2024

I've created a PR at emscripten-core/emscripten#21964, as I don't believe this change will impact other functionality and could be useful for others working across file systems. I'll follow the contributor guidelines and ask for help in their Discord, too.

@brandonpayton
Copy link
Member

Have Playground contributors had success in getting PRs approved in the Emscripten repository?

Thankfully, yes! Here is a recent example:
emscripten-core/emscripten#21805

@SiobhyB
Copy link
Author

SiobhyB commented May 20, 2024

@brandonpayton, awesome! I also found this recent example from @fluiddot, so my fingers are crossed!

@brandonpayton
Copy link
Member

Nice! 🤞 Every interaction I've had with that team has been great, with surprisingly fast feedback.

@SiobhyB SiobhyB closed this May 23, 2024
@SiobhyB SiobhyB deleted the fix/ensure-wordpress-updates-alway-complete branch May 23, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants