fix: make copy deploy symlink-aware and surface deploy errors - #39
Open
ssx wants to merge 2 commits into
Open
Conversation
The copy deploy strategy was not symlink-aware. When a mapped destination is a symlink whose target does not currently exist (common with shared persistent storage, e.g. pub/media/custom_options -> /shared/...), file_exists() follows the link and reports the path as absent, so mkdir() runs against an existing link node and fails with "mkdir(): File exists". The resulting ErrorException aborts the rest of that package's deploy. DeployManager::doDeploy() only logged that exception in debug mode, so the failure was silent at normal verbosity, leaving directories such as setup/ partially or completely unpopulated with no diagnostic output. - Copy::createDelegate(): preserve a pre-existing symlink at the destination instead of trying to mkdir over it. - DeployManager::doDeploy(): write deploy failures as a warning at normal verbosity instead of only in debug mode.
5 tasks
Author
|
This is a fix for the issue reported here: magento/magento2#40864 |
Author
|
@magento I have signed the Magento Contributor Agreement |
Follow-up to the symlink-aware deploy change. A failed file deploy leaves Magento unable to run, so it should abort the install loudly rather than continue. - DeployManager::doDeploy(): re-throw deploy failures (previously swallowed outside debug mode) as a RuntimeException that names the package, the underlying message, and the originating file:line. - Copy::createDelegate(): when the destination exists but is not a directory (and is not a symlink to preserve), throw an ErrorException naming the exact path instead of letting mkdir() emit a path-less "File exists" warning. Complies with the Magento2 coding standard (no silenced errors; the remaining discouraged-filesystem-function warnings are pre-existing and unavoidable in a Composer plugin, where Magento\Framework\Filesystem is not available).
Author
|
@magento-engcom-team is anyone alive that checks this repo? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The
copydeploy strategy is not symlink-aware, andDeployManagersilently swallows deploy failures outside debug mode. Together these cause Magento installs to end up with a partially or completely unpopulatedsetup/directory (and other mapped dirs) with no error output.Root cause
Copy::createDelegate()(dir-to-dir branch):When a mapped destination is a symlink whose target does not currently exist (common with shared persistent storage, e.g.
pub/media/custom_options -> /shared/pub/media/custom_options),file_exists()follows the link and reports the path as absent.mkdir()then runs against an existing link node and fails withmkdir(): File exists.DeployManager::doDeploy()caught the resulting\ErrorExceptionbut only wrote it in debug mode, so the package's remaining mappings (includingsetup/) were skipped silently.Fix
Copy::createDelegate()is symlink-aware — a pre-existing symlink at the destination (e.g. a shared-storage mount) is preserved instead of being clobbered ormkdir-ed over. A destination that exists but is not a directory (and is not a symlink) now throws an\ErrorExceptionnaming the exact path, rather than lettingmkdir()emit a path-less warning.DeployManager::doDeploy()hard-fails — a deploy error is no longer swallowed. It is re-thrown as a\RuntimeExceptionnaming the package, the underlying message, and the originatingfile:line, aborting the install with a non-zero exit. A broken/incomplete deploy means the application cannot run, so failing loudly is correct.Legitimate shared-storage symlinks are preserved (not a failure); only genuine deploy errors abort.
Manual testing
On a clean
magento/project-community-edition=2.4.8-p5install (defaultcopystrategy):find setup -type f | wc -l→566(baseline).ln -s /shared/pub/media/custom_options pub/media/custom_options(dangling target), thencomposer reinstall magento/magento2-base.setup/empty/missing, no error printed;-vvvrevealsmkdir(): File existsatCopy.php.setup/back to566, and the symlink is preserved.Coding standard
Verified clean under the Magento2 PHPCS standard (no silenced errors). The remaining
DiscouragedFunctionwarnings onmkdir/is_dir/is_link/file_existsare pre-existing throughout this file and unavoidable here —Magento\Framework\Filesystem\DriverInterfaceis not available at Composer-plugin runtime.Notes
Related upstream report filed at magento/magento2 issues.