Skip to content

fix: resolve path inconsistencies in MakeManualSiteNamesJson / MakeManualSiteNamesJsonのパス解決不整合を修正 - #593

Merged
zigzagdev merged 3 commits into
refactor/console-commands-cleanupfrom
refactor/console-commands-cleanup_manual-names-path
Oct 20, 2026
Merged

zigzagdev merged 3 commits into
refactor/console-commands-cleanupfrom
refactor/console-commands-cleanup_manual-names-path

Conversation

@zigzagdev

Copy link
Copy Markdown
Owner

Motivation / 目的

While investigating a candidate for the console-command path-resolution cleanup, MakeManualSiteNamesJson was found to have its own divergent resolvePath()/toLocalDiskPath() implementations instead of using the shared ResolvesLocalDiskPaths trait. Investigating why revealed two real bugs, not just duplication.

コンソールコマンドのパス解決共通化のリファクタ候補を調査する中で、MakeManualSiteNamesJsonだけが独自resolvePath()/toLocalDiskPath()を持っていることが分かりました。単なる重複ではなく、実際に2つの不具合が
見つかりました。

What I have done / 実施内容

  • Adopted ResolvesLocalDiskPaths for --in: resolvePath() used base_path() directly, which only happened to work because the documented default already includes the full storage/app/private/...prefix. Unlike every other command in this pipeline, it could not accept a disk-relative path (e.g. --in=unesco/manual_input.txt), silently resolving to the wrong location under the project root.
  • Fixed a doubled private/ segment bug on --out: toLocalDiskPath() only stripped the storage/app/ prefix, not private/. With the documented default (storage/app/private/unesco/site_names_manual.json), the file actually landed at storage/app/private/private/unesco/site_names_manual.json (local disk root is storage/app/private), while the log line claimed the correct single-private path. Switched to ResolvesLocalDiskPaths::normalizeLocalDiskPath() and fixed the log line to print the real resolved path

Test Results / テスト結果

  • php artisan test (full suite): 228 passed, no regressions
  • MakeManualSiteNamesJsonTest: confirms --in accepts a disk-relative path, and --out no longer lands under a doubled private/ segment
  • Verified end-to-end against real fixture files in the container before writing the tests

resolvePath() used base_path() directly, which only worked because the
--in default option already includes the full storage/app/private/...
prefix. Unlike every other console command in this pipeline, it could
not accept a disk-relative path (e.g. --in=unesco/manual_input.txt),
silently resolving it to the wrong location under the project root.

ResolvesLocalDiskPaths::resolvePathToFile() handles both forms
correctly and resolves to the identical path for the existing default
option value.
…on output

toLocalDiskPath() only stripped the storage/app/ prefix, not private/.
With the documented default --out value
(storage/app/private/unesco/site_names_manual.json), the file actually
landed at storage/app/private/private/unesco/site_names_manual.json on
the local disk (root storage/app/private), while the log line claimed
the correct single-private path.

Switch to ResolvesLocalDiskPaths::normalizeLocalDiskPath(), which
strips both prefixes, and log the real resolved path instead of a
hardcoded "storage/app/" prefix assumption.
Regression tests for both fixes:
- --in now accepts a disk-relative path like every other command
- --out no longer lands under a doubled private/ segment
@codecov

codecov Bot commented Oct 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.06%. Comparing base (8eba9d2) to head (420ef18).

Additional details and impacted files

Impacted file tree graph

@@                           Coverage Diff                           @@
##             refactor/console-commands-cleanup     #593      +/-   ##
=======================================================================
+ Coverage                                67.72%   69.06%   +1.33%     
+ Complexity                                1691     1687       -4     
=======================================================================
  Files                                      151      152       +1     
  Lines                                     8912     8915       +3     
=======================================================================
+ Hits                                      6036     6157     +121     
+ Misses                                    2876     2758     -118     
Files with missing lines Coverage Δ
...c/app/Console/Commands/MakeManualSiteNamesJson.php 77.86% <100.00%> (+77.86%) ⬆️
...orldHeritage/Tests/MakeManualSiteNamesJsonTest.php 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zigzagdev zigzagdev changed the title Refactor/console commands cleanup manual names path fix: resolve path inconsistencies in MakeManualSiteNamesJson / MakeManualSiteNamesJsonのパス解決不整合を修正 Oct 20, 2026

@zigzagdev zigzagdev left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@zigzagdev
zigzagdev merged commit 175f8c1 into refactor/console-commands-cleanup Oct 20, 2026
28 checks passed
@zigzagdev
zigzagdev deleted the refactor/console-commands-cleanup_manual-names-path branch October 20, 2026 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant