Skip to content

BasePHP: Use FS.rename for mv method#445

Merged
adamziel merged 2 commits intoWordPress:trunkfrom
eliot-akira:base-php-use-fs-rename-for-mv-method
May 25, 2023
Merged

BasePHP: Use FS.rename for mv method#445
adamziel merged 2 commits intoWordPress:trunkfrom
eliot-akira:base-php-use-fs-rename-for-mv-method

Conversation

@eliot-akira
Copy link
Copy Markdown
Collaborator

@eliot-akira eliot-akira commented May 24, 2023

What?

Update BasePHP.mv method to use FS.rename instead of FS.mv which doesn't exist.

Why?

The FS method is called rename, not mv, in both Emscripten and Node.js.

Testing Instructions

  1. Check out the branch.
  2. Run npx nx test php-wasm-node.

@adamziel
Copy link
Copy Markdown
Collaborator

This is great @eliot-akira, thank you so much for finding and fixing this at the framework level – this benefits every Playground project!

@adamziel
Copy link
Copy Markdown
Collaborator

Would you also add a unit test to prevent any future regressions? They live in php-wasm/node/src/test/php.spec.ts for now.

@eliot-akira
Copy link
Copy Markdown
Collaborator Author

Sure, I added the following tests:

  • mv() should move a file
  • mv() should replace target file if it exists
  • mv() should throw a useful error when source file does not exist
  • mv() should throw a useful error when target directory does not exist

@adamziel
Copy link
Copy Markdown
Collaborator

10/10 💯

@adamziel adamziel merged commit 07700f0 into WordPress:trunk May 25, 2023
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.

2 participants