Add symlink support to Composer Stager 2.0, require that version, and simplify UX & tests accordingly

Reflecting the real current scope.

We'll need to drastically change SymlinkValidatorTest, but we'll also need to update docs & tooling. This only does the latter.

Yay, @TravisCarden has a PR ready, and I just posted an initial review: https://github.com/php-tuf/composer-stager/pull/61#pullrequestreview-131... 😊

Exciting progress there: https://github.com/php-tuf/composer-stager/pull/61#issuecomment-1446750505 🚀

AFAICT it's at a point where we should start running tests here using that PR instead of

"php-tuf/composer-stager": "^1.2",

The PR (https://github.com/php-tuf/composer-stager/pull/61) has been reviewed and is ready to be tested with the module here. Let me know how I can help.

⚠️ When we do this, we'll also need to add symlink-related edge case tests to all our validators!

At a minimum, these validators need to get additional test coverage (and likely additional logic):

• GitExcluder: excludes .git except if the installed composer package is installed via git → I don't think we can eliminate the possibility of a symlink somewhere along the way
• NodeModulesExcluder node_modules directory should be ignored everywhere → same
• DuplicateInfoFileValidator → same

Okay, I'm not worried about the test failures in the most recent commit. That's just straight-up due to the branch name, as far as I can tell. Everything else passes.

What I'm stuck on is...I don't understand what Wim is asking for in #26. What would these scenarios look like?

That MR looks great! 🤩

\Drupal\Tests\package_manager\Kernel\SymlinkValidatorTest::testSymlink() currently just simulates assertIsFulfilled() returns TRUE.

IMO we should change this to a proper kernel test (the current one is pretty much a unit test) that proves that our assumptions hold true: composer-stager should:

1. not complain about intra-package symlinks: simulate the Drush example, because that's very common
2. not complain about inter-package symlinks (I first thought we would not support this but @TravisCarden articulated why it would be fine, so 👍)
3. complain about a hard link
4. complain about a symlink to an absolute target
5. complain about a symlink in active to a relative target outside active
6. complain about a symlink in stage to a relative target outside stage

Right now we're delegating to composer-stager's test coverage. That's dangerous, because its assumptions could change. Having this test would defend Drupal core against (accidental or intentional) upstream changes that could otherwise brick Drupal sites.

The above would not test the edge cases. That's still up to composer-stager's tests to test (and it does!) — it would just test the high-level categories. And that I think is very much worth doing given the big potential consequences.

The failures do seem innocent, but they still worry me, because it's exactly the tests that we absolutely want to see pass: the end-to-end build tests! You did specify the dev- prefix like https://getcomposer.org/doc/articles/versions.md#branches prescribes, yet it still failed … 🤔 It's really weird that the error output says ~dev-feature/symlink-support though, with a ~ prefixed, which we did not do … or did we? \Drupal\automatic_updates\ReleaseChooser::getMostRecentReleaseInMinor() seems to do something like this… although IDK why it'd be called for anything other than Drupal core?!

Also missing from the MR: the docs changes that I had already posted in #22.

In #26 I wrote:

At a minimum, these validators need to get additional test coverage (and likely additional logic):

• GitExcluder: excludes .git except if the installed composer package is installed via git → I don't think we can eliminate the possibility of a symlink somewhere along the way
• NodeModulesExcluder node_modules directory should be ignored everywhere → same
• DuplicateInfoFileValidator → same

The DuplicateInfoFileValidator one is no longer relevant, because it now reuses core's ExtensionDiscovery, meaning it already finds the exact same *.info.yml files that core would, because it reuses the exact same search logic 👍

That leaves only GitExcluder & NodeModulesExcluder. Both of those currently do not follow symlinks. Which means that composer-stager will find matches in the following project layout, but those 2 excluders won't:

$ tree -a
.
├── composer.json
├── composer.lock
├── custom
│   └── MYMODULE
│       ├── .git
│       ├── MYMODULE.info.yml
│       └── node_modules
├── index.php
├── modules
│   └── MYMODULE -> ../custom/MYMODULE
└── vendor

👆 This is not a common layout obviously, but we need to make sure all edge cases are covered, because there's nothing illegal about it either. Because those 2 excluders do not follow symlinks, they stop searching at the modules/MYMODULE point, even though the *.info.yml file will be discovered by ExtensionDiscovery (so Drupal will happily discover this module and use it, and our excluder will also correctly spot it). But custom/MYMODULE/node_modules will not be ignored as it should be, nor will custom/MYMODULE/.git.

Precisely because this was previously unsupported (composer-stager disallowed ALL symlinks), we didn't have test coverage for it. But this MR will add support for it, so we need to ensure it actually works, end-to-end, and that it's not our code that is making Package Manager fail when symlinks are used, rather than Composer Stager!

Tagging for this as well as #30.

not complain about intra-package symlinks: simulate the Drush example, because that's very common

✅

not complain about inter-package symlinks

✅, but 🤔...this is really the same, if you look at it, as intra-package symlinks. The validator has no awareness of packages, or the boundaries between them. So IMHO this is not really adding anything and can be removed.

complain about a hard link

🐞 🐛 Composer Stager does something wrong here. Several of the more specific preconditions that check for unsupported symlinks will throw an IOException if the path they get is not a symlink (i.e., they will throw if they're given a hard link). This would normally not be an issue, except that the precondition that checks for hard links does NOT run first, and therefore these other preconditions might hit the hard link first and throw an exception.

The proper fix here is upstream; \PhpTuf\ComposerStager\Infrastructure\Aggregate\PreconditionsTree\NoUnsupportedLinksExist::__construct() needs to put $noHardLinksExist first.

complain about a symlink to an absolute target

✅

complain about a symlink in active to a relative target outside active

✅

complain about a symlink in stage to a relative target outside stage

✅

✅, but 🤔...this is really the same, if you look at it, as intra-package symlinks. The validator has no awareness of packages, or the boundaries between them. So IMHO this is not really adding anything and can be removed.

Hm … 🤔 But that's only because you and I know that the current upstream logic does not distinguish between the two. That doesn't mean it never will.

I personally don't see a valid real-world use case for inter-package symlinks though, I'm mostly concerned about intra-package symlinks, because those are fairly common!

Conclusion: 👍

🐞 🐛 Composer Stager does something wrong here.

Hah … so this is why I sort of was wanting to have that test written here before the upstream PR was merged 🙈 But … no big deal of course! We can get that fixed upstream 😊 Can you report it? 🙏

Hah … so this is why I sort of was wanting to have that test written here before the upstream PR was merged 🙈 But … no big deal of course! We can get that fixed upstream 😊 Can you report it? 🙏

Eh, I was pretty sure we'd encounter at least one small thing. It's literally a one-line fix, though, so that's still a pretty excellent outcome.

It is!!! 😄

Clarifying #31: the thing we need to check here is that, if any .git or node_modules directory are existing under a symlink, they are still detected and excluded.

Filed a specific issue for the hard link bug: https://github.com/php-tuf/composer-stager/issues/98

And https://github.com/php-tuf/composer-stager/issues/99 for the symlinks-to-directories weirdness.

Status update:

• https://github.com/php-tuf/composer-stager/issues/98 landed
• https://github.com/php-tuf/composer-stager/issues/99 did not yet land.

@phenaproxima's recent commits made this pass tests. AFAICT that means it's now feasible to already work on the test coverage mentioned before?

Well...not quite.

I have yet to get the full details on this, but according to @TravisCarden:

• We are trying to test symlinks that point to directories.
• It's not working.
• Composer Stager has no test coverage for that either.
• When Travis tried to add coverage for that, he ran into some kind of rabbit hole that he says is very complicated and will take a while to fix.
• So we might have to scale back what we've got here.

Point is, this may be somewhat dragon-shaped. Will update when I have a clearer picture of where we stand.

From a conversation with @TravisCarden in Slack:

the problem with directory symlinks, as I currently understand it, is that if you copy() a symlink to a file, it copies the symlink (the source). But if you copy() a symlink to a directory, it tries to copy the directory it points to (the destination). More precisely, it refuses to try to copy it because, to quote the error, The first argument to copy() function cannot be a directory. My current thought is that in order to do this, for every file in the directory tree, we'll have to do all of the following:

1. Ask if the file is a symlink. (Disk hit.)
2. If it is, ask if it points to a directory. (Disk hit.)
3. If it does, ask what directory it points to. (Disk hit.)
4. Check whether it's a supported type, i.e., relative, within the codebase.
5. If it is supported recursively delete the target directory, because rmdir() only works on an empty directory. (Disk hit, disk hit, disk hit...)
6. Delete the directory. (Disk hit.)
7. Finally, instead of copying the symlink, create a new one in the destination. (Disk hit, obviously.)

So it can be done, but obviously, it'll be a lot of engineering effort, and it'll adversely impact performance.

After some discussion, here's the plan.

• @TravisCarden will add an additional precondition in Composer Stager which scans for symlinks to directories, and complains if it finds any. We're postponed until that's done.
• When it's done, though, we'll bring that change in, and add test coverage on our end.
• We'll create a decorated version of that precondition which exits without complaint if Package Manager is explicitly configured to use the rsync file syncer, since symlinks to directories pose no problem for rsync.

This will net us broad support for common symlinking scenarios (including the Drush problem), with very clear rules about what kind of symlinks aren't supported. That's good enough to land Package Manager in core as alpha experimental.

Given the complexity detailed in #43, it's unclear if we'll want to add support for directory symlinks later on. If we do, here's one way we could do it in Package Manager, for sites that don't have rsync:

• On pre-apply, scan the staging directory for any symlinks to directories within the codebase.
• Store that information - the paths of the symlinks, and their relative targets.
• On post-apply, re-create all of those symlinks.

However, if we do this, it's not an alpha blocker, and we can detail it in a follow-up later.

Thanks for the detailed update! 👍

Travis merged the change I detailed in #44, so this is no longer blocked.

The test coverage looks great! 🤩👏

Posted some questions, but nothing serious 😊

BTW, #44 also explains 🐛 [upstream] Updates fail if OS temp directory is a symlink Postponed … 😅

I think this is reviewable now.

99% ready! 👏

Epic work on improving those tests, they're crystal-clear now! 🤩🤩🤩

This still should apply most of that patch I posted in #22. Although it looks like based on the "symlink to directory" problem that remains, we should still have some hook_help() entry instead of none at all…

Oooh, good catch. Yeah, I'll apply the code from #22 and also ensure we're linking to the relevant documentation in Composer Stager's code base.

Applying patch #22 means that all symlink-related documentation is removed from Package Manager's hep. Which means SymlinkValidator no longer needs to link to the online help if it finds problems. The individual parts of Composer Stager's preconditions are clear and explicit enough that I don't think any documentation is needed!

This greatly simplifies SymlinkValidator -- it's now just a wrapper around the precondition, nothing more -- and allows me to remove the test coverage for its linking to the online help. 🎉

Back to Wim to confirm everything looks right.

Even though I think it would've been nice to see explicit docs for the "symlinks to dirs" not supported edge case, that is going away eventually, and most people now actually won't run into this.

So, I think that overall, this MR is ready. 👏

Of course, it cannot land until php-tuf/composer-stager tags 2.0.0-alpha1. So marking RTBC but indicating in the title that it cannot be committed yet. Also, it MUST be @tedbow who commits this, since @phenaproxima did 99% of the work.

While at it, let's also make the issue title actually accurate of the current direction & reality 😊

Actually postponing this, just so nobody accidentally commits it or counts it as a "true" RTBC.

On second thought, this will need to be updated when Composer Stager tags, so maybe it's just plain postponed.

OK, found another bug while manually testing this. Luckily it's not too bad, but it does need to be fixed upstream.

(The problem is that symlink targets are read and resolved relative to the current working directory, not the directory in which the symlink itself lives, which produces false positives.)

Composer Stager 2.0.0-alpha1 has been tagged, so this is now unblocked.

Finally.

🥳

Wow, just seeing this. Feel free to reach out to me in the future. I might be grumpy sometimes but i do support this initiative.

@moshe weitzman So … this means you're happy to see this issue get fixed? 😊🤞

Automatically closed - issue fixed for 2 weeks with no activity.