From 46a029b338931fd76c8fc44fc13c5774b3657af0 Mon Sep 17 00:00:00 2001 From: nsfisis Date: Fri, 24 Jul 2026 01:25:16 +0900 Subject: docs(symfony-filesystem): mark unported Filesystem gaps with TODO(phase-c) Audited every remaining method in the Symfony Filesystem port against composer/vendor/symfony/filesystem/Filesystem.php and tagged each divergence with a searchable TODO(phase-c): the missing self::$lastError propagation, copy()'s collapsed fopen-failure messages and skipped mtime preservation, exists()'s missing PHP_MAXPATHLEN guard, do_remove()'s unported rename/rollback safety trick and its Unix short-circuit gap, symlink()'s unported Windows path-normalization/copy_on_windows fallback, link_exception()'s unported error-code-1314 message, read_link()'s missing canonicalize=true overload and its Windows PHP<7.4 quirk, and mirror()'s missing getRealPath()/filesCreatedWhileMirroring dedup. Windows-only branches were previously left with plain comments claiming they "never run on Unix" instead of the required TODO marker, which understates them as permanently out of scope rather than unported work. Co-Authored-By: Claude Sonnet 5 --- .../src/symfony/filesystem/filesystem.rs | 70 +++++++++++++++++++--- 1 file changed, 62 insertions(+), 8 deletions(-) (limited to 'crates/shirabe-external-packages/src') diff --git a/crates/shirabe-external-packages/src/symfony/filesystem/filesystem.rs b/crates/shirabe-external-packages/src/symfony/filesystem/filesystem.rs index 9b449b76..d773e60c 100644 --- a/crates/shirabe-external-packages/src/symfony/filesystem/filesystem.rs +++ b/crates/shirabe-external-packages/src/symfony/filesystem/filesystem.rs @@ -1,5 +1,10 @@ //! ref: composer/vendor/symfony/filesystem/Filesystem.php +// TODO(phase-c): PHP's box()/self::$lastError mechanism (captures the underlying OS error message +// from a failed native call via set_error_handler) is not modeled anywhere in this file. Every +// IOException message constructed below therefore omits the trailing low-level error string that +// PHP would append (e.g. "Failed to touch \"%s\": ".self::$lastError). + use crate::symfony::filesystem::exception::io_exception::IOException; use shirabe_php_shim::PhpMixed; @@ -77,6 +82,10 @@ impl Filesystem { let bytes_copied = match shirabe_php_shim::copy(origin_file, target_file) { true => shirabe_php_shim::filesize(target_file).unwrap_or(0), false => { + // TODO(phase-c): PHP distinguishes fopen($originFile) failure ("source file + // could not be opened for reading") from fopen($targetFile) failure ("target + // file could not be opened for writing"); the shim's copy() collapses both + // (and the actual copy failure) into this single generic message. return Err(IOException::new( format!("Failed to copy \"{}\" to \"{}\".", origin_file, target_file), 0, @@ -106,8 +115,9 @@ impl Filesystem { as u32, ); - // Like `cp`, preserve the file modification time. The shim's touch2/touch3 (explicit - // mtime) are unimplemented (no utimensat), so mtime preservation is omitted here. + // TODO(phase-c): PHP preserves the origin file's mtime via + // touch($targetFile, filemtime($originFile)). shirabe_php_shim::touch2 now exists + // and could implement this, but it is not wired up here yet. let bytes_origin = shirabe_php_shim::filesize(origin_file).unwrap_or(0); if bytes_copied != bytes_origin { @@ -148,6 +158,9 @@ impl Filesystem { } fn exists(&self, files: PhpMixed) -> bool { + // TODO(phase-c): PHP throws IOException when a path exceeds PHP_MAXPATHLEN - 2 characters; + // this port has no such guard, and the plain `bool` return type here cannot express that + // throw path without changing the signature. for file in Self::to_iterable(&files) { if !shirabe_php_shim::file_exists(&file) { return false; @@ -161,6 +174,11 @@ impl Filesystem { Self::do_remove(files, false) } + // TODO(phase-c): `is_recursive` is unused. In PHP, doRemove() uses it as a top-level-call guard: + // on the first (non-recursive) call for a directory, it renames the directory to a random + // hidden name before recursing into it (and renames it back if rmdir subsequently fails), to + // avoid a race where another process recreates the path mid-removal. That rename/rollback + // trick is entirely unported here; this version always operates on the original path. fn do_remove(files: Vec, is_recursive: bool) -> anyhow::Result<()> { // PHP reverses the list so that directory contents are removed before the directory itself. let mut files = files; @@ -168,6 +186,12 @@ impl Filesystem { for file in files { if shirabe_php_shim::is_link(&file) { // See https://bugs.php.net/52176 + // TODO(phase-c): PHP's condition is + // `!(unlink() || '\\' !== DIRECTORY_SEPARATOR || rmdir()) && file_exists()`. On + // Unix, `'\\' !== DIRECTORY_SEPARATOR` is always true, so the `!(...)` is always + // false and this branch never throws there regardless of unlink's result (the + // rmdir fallback and this exception only matter on Windows). This port omits that + // always-true disjunct, so it CAN throw here on Unix where upstream never would. if !shirabe_php_shim::unlink(&file) && shirabe_php_shim::file_exists(&file) { return Err(IOException::new( format!("Failed to remove symlink \"{}\": ", file), @@ -211,6 +235,10 @@ impl Filesystem { .into()); } } else if !shirabe_php_shim::unlink(&file) && shirabe_php_shim::file_exists(&file) { + // TODO(phase-c): PHP also throws when self::$lastError contains "Permission + // denied", even if file_exists() is now false (e.g. the file vanished between the + // failed unlink and this check). That OR-branch is dropped along with the general + // $lastError omission noted at the top of this file. return Err(IOException::new( format!("Failed to remove file \"{}\": ", file), 0, @@ -223,14 +251,16 @@ impl Filesystem { Ok(()) } + // TODO(phase-c): PHP's symlink() has a `'\\' === DIRECTORY_SEPARATOR` branch that (a) + // normalizes '/' to '\\' in both paths, and (b) when $copyOnWindows is true, mirrors the + // directory instead of symlinking it and returns early. Neither is ported: `_copy_on_windows` + // is accepted but unused, so this always symlinks even where PHP would have copied. pub fn symlink( &self, origin_dir: &str, target_dir: &str, _copy_on_windows: bool, ) -> anyhow::Result<()> { - // On Unix DIRECTORY_SEPARATOR is '/', so the Windows-only path normalization and - // copy-on-windows branch never run. self.mkdir( PhpMixed::String(shirabe_php_shim::dirname(target_dir)), 0o777, @@ -249,8 +279,11 @@ impl Filesystem { Ok(()) } + // TODO(phase-c): PHP special-cases a Windows error containing "error code(1314)" with a + // distinct "Do you have the required Administrator-rights?" message; that check (and the + // self::$lastError inspection it depends on) is not ported, so this always throws the generic + // message below. fn link_exception(origin: &str, target: &str, link_type: &str) -> anyhow::Result<()> { - // The Windows error-code-1314 branch never runs on Unix. Err(IOException::new( format!( "Failed to create \"{}\" link from \"{}\" to \"{}\": ", @@ -263,10 +296,20 @@ impl Filesystem { .into()) } + // TODO(phase-c): this only ports Symfony's readlink($path, $canonicalize = false) overload; + // the $canonicalize = true branch (realpath()-based resolution, returning null if the path + // does not exist at all) is entirely unported. Per the phase B default-argument convention + // this should become a `read_link2` overload if that branch is ever needed. fn read_link(&self, path: &str) -> String { // Symfony's readlink() with $canonicalize = false: returns null if the path is not a link. - // The Rust signature is non-Option, so the non-link case yields the path's readlink result - // (empty string on failure) to keep the symlink() caller working. + // TODO(phase-c): the Rust signature is non-Option, so the non-link case yields the path's + // readlink result (empty string on failure) instead of PHP's null, to keep the symlink() + // caller working. Every current caller checks is_link() first, so this never triggers on + // the live code paths, but the collapsed Option -> String signature is a real + // divergence from upstream. + // TODO(phase-c): PHP also has `if ('\\' === DIRECTORY_SEPARATOR && PHP_VERSION_ID < 70400) + // return realpath($path);` ahead of the plain readlink() call below, working around a + // pre-7.4 Windows bug. Not ported here; on old Windows PHP this would resolve differently. std::fs::read_link(path) .ok() .map(|p| p.to_string_lossy().into_owned()) @@ -274,7 +317,11 @@ impl Filesystem { } // PHP stream_is_local(): true for plain paths and the file:// wrapper, false for remote - // wrappers (http://, ftp://, ...). Approximated via the URL scheme since there is no shim. + // wrappers (http://, ftp://, ...). + // TODO(phase-c): this is PHP's built-in stream_is_local(), which queries the registered stream + // wrapper for STREAM_IS_URL rather than just parsing the scheme. It is approximated here via + // parse_url()'s scheme instead of being added to shirabe-php-shim, so a registered custom + // stream wrapper claiming to be local (or vice versa) would be classified differently than PHP. fn stream_is_local(path: &str) -> bool { let scheme = shirabe_php_shim::parse_url(path, shirabe_php_shim::PHP_URL_SCHEME); match scheme { @@ -366,6 +413,13 @@ impl Filesystem { .and_then(|v| v.as_bool()) .unwrap_or(false); + // TODO(phase-c): PHP's skip condition is + // `$file->getPathname() === $targetDir || $file->getRealPath() === $targetDir || + // isset($filesCreatedWhileMirroring[$file->getRealPath()])`, and it records every + // `$target` it creates into `$filesCreatedWhileMirroring` to avoid revisiting a path + // already produced earlier in this same mirror() call (e.g. via a symlink loop back into + // the tree). Only the plain pathname comparison is ported; the getRealPath() comparison + // and the created-files dedup set are both omitted. for file in &iterator { let pathname = file.get_pathname(); if pathname == target_dir { -- cgit v1.3.1