Problem/Motivation
Composer Stager provides a precondition to detect the presence of symlinks in the code base. Right now, our symlink validator does its own detection of symlinks, but we should delegate to Stager's mechanism to keep things nice and consistent.
Issue fork automatic_updates-3298444
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
tedbowHere is the merge request in Composer Stager where the functionality was added https://github.com/php-tuf/composer-stager/pull/38
Our current validator is \Drupal\package_manager\Validator\SymlinkValidator
Comment #3
phenaproximaI've been messing with this and the trouble I'm running into is that, although it's very easy to delegate the check to the Composer Stager precondition, it comes at the cost of a less clear error message.
The precondition expects to be passed an active directory and a staging directory, and it checks both. If either one contains a symlink, it comes back with "The code base contains symlinks". It doesn't get any more specific than that.
That's a problem because, right now, our validator is a lot clearer about whether the active directory has symlinks, or the staging area does (or both). That's inherently more actionable and better for users. But, due to the way Composer Stager's preconditions are architected, I'm not sure it'll be possible to easily override this without, well, re-implementing a bunch of the precondition's internal logic. Which, of course, defeats the point of delegating to it in the first place.
As a workaround, I could just pass the same path twice, and call the precondition multiple times. But that will incur a performance penalty, since the directory will be scanned twice. To get around that, I could add a static caching layer so that we don't do expensive filesystem checks multiple times.
I'd like @TravisCarden's input on what to do here. He may have some brilliant idea I've overlooked.
Comment #4
tedbowIt looks like if you sent the stage has something that just exists it would just send you the active
But I guess that would assuming the internals
Comment #5
tedbowAlso would it be useful for Composer Stager itself to have 2 pre-conditions 1 for stage and 1 for active instead of just `CodeBaseContainsNoSymlinks`? Right now if you got the message "'The codebase contains symlinks.'" it doesn't tell you where to start looking
Comment #6
phenaproximaI agree; I suspect that we could have something like DirectoryContainsNoSymlinksInterface, and an additional interface that allows us to set the message returned by an unfulfilled precondition. Or, even just having the "set the message first" interface would probably fix this problem for us.
I guess we can see how @TravisCarden feels about that.
Comment #7
traviscarden commentedAs to a temporary workaround, @phenaproxima, a less expensive option would be verify the precondition twice, once for the active directory and once for the staging directory, but pass in a known empty directory (or even
__DIR__) for the second path.Obviously, Composer Stager itself should be enhanced to provide the detail you need. @tedbow's suggestion of decomposing
CodeBaseContainsNoSymlinksintoActiveDirContainsNoSymlinksandStagingDirContainsNoSymlinkswould probably be the cleanest solution if you want to actually condition logic on the difference.DirectoryContainsNoSymlinksInterfaceis a possibility, too, though I'd want to consider the relative merits. If you just want more detail in the error message, that could be done without adding classes at all. Give me your thoughts. And please provide an example of an error message you'd like to get back from the API.Comment #8
tedbowI don't think it is urgent to get this issue done. especially if we do a work around like sending a known empty directory. Because if composer stager is going to make a change then we will just have to redo this validator and test again. Our current validator works it just makes sense for the logic to be in Composer stager in the long term
Comment #9
phenaproximaAll I want is more detail in the error message :) Right now, "The codebase contains symlinks" is too vague. I'd prefer something like "The active directory contains symlinks." Or, better yet, "The active directory (/path/to/active/directory) contains symlinks, which is not supported."
Comment #10
traviscarden commentedDone: Add detail to CodeBaseContainsNoSymlinks error message by TravisCarden · Pull Request #45 · php-tuf/composer-stager
Comment #13
omkar.podey commentedChanged Implementation Because Beginner::setFixturePath(NULL) won't do anything as the paths have already been set and create() uses pathlocator to get active_dir path which is pointing to a temp directory which has the unreadable directory.
Comment #14
omkar.podey commentedComment #15
traviscarden commentedComment #16
traviscarden commentedComment #17
omkar.podey commentedComment #18
traviscarden commentedLooks good!
Comment #20
phenaproximaThanks! Merged into 8.x-2.x.
Comment #22
tedbow