Address file permission issues in new directories created by WordPress on Windows - #66
Conversation
Instead of modifying the `chmod` and `mkdir` functions, the handler used when invoking `stat` is updated to ensure it returns correct permissions on Windows. This approach mirrors a workaround applied in `NODEFS.getMode`, where the execution bit is represented based on the read permission. Reference: https://github.com/emscripten-core/emscripten/blob/63c648fa17be49fa9640d4a40c8ba2f7a9b2d444/src/library_nodefs.js#L71-L75
| @@ -29535,6 +29550,11 @@ function init4(RuntimeName, PHPLoader) { | ||
| doStat: function(func, path, buf) { | ||
| try { | ||
| var stat = func(path); | ||
| + if (NODEFS.isWindows) { | ||
| + // Node.js on Windows never represents permission bit 'x', so | ||
| + // propagate read bits to execute bits. | ||
| + stat.mode = stat.mode | (stat.mode & 292) >> 2; | ||
| + } | ||
| } catch (e) { | ||
| if (e && e.node && PATH.normalize(path) !== PATH.normalize(FS.getPath(e.node))) { | ||
| return -54; |
There was a problem hiding this comment.
This corresponds to PHP version 8.0, the one that is used by default in the Studio app.
|
Thanks for the fix @fluiddot ! It works well for me on macOS. I didn't test on Windows, though. Should we implement this fix in the wordpress-playground repository? |
sejas
left a comment
sejas
left a comment
There was a problem hiding this comment.
Wow, great investigation in finding the permissions error! 👏
Yes, I think we should create a PR for php-wasm/node so the broader community can benefit from this fix.
Yes, although ideally, we should apply the fix directly in the Emscripten repository. This way we'll ensure all PHP versions will have the fix. I'll work on a PR for this. In the meantime, I can apply the same workaround in
Yes, I agree. This will address issues on Windows when using |
|
I created a PR in Emscripten to solve this problem as a long-term solution. It might take some time until it lands in a new version, so as shared here, I'll work on a fix in Playground. Once that's ready and merged, we could create another PR for the Studio app and remove this patch. That said, since this PR solves the issue, I'm merging it. |
3 participants
Related to #45.
Proposed Changes
Patch PHP runtimes to return correct file permissions on Windows, specifically focusing on the
doStatfunction, which acts as the handler when invokingfs.stat,fs.lstat, andfs.statfs. The concept behind this patch is to replicate a similar workaround applied in Emscripten for NODEFS (commit reference) to deduce the missing executable bit (x) on Windows, by leveraging the read bit (r).This adjustment will ensure that WordPress accurately retrieves the file permissions from the parent directory when creating different directories during media uploads (reference). Currently when getting the file permissions of a directory on Windows via PHP's
statfunction, it returns666. The fact that the executable bit is missing (x), leads to WordPress not being able to access the directories created although it has permission.Note
Note that the adjustments need to be made for every supported PHP version.
Testing Instructions
Windows
macOS
npm starton a macOS machine.Pre-merge Checklist