Problem/Motivation

Talking with @bnjmnm he brought up the fact that the node_modules folder will often have symlinks. So our SymlinkValidator will stop operations if the node_modules folder is present. This is pain for Project Browser development

Right now don't have a listener that excludes node_modules but if we added one like the others in package_manager/src/PathExcluder that excluded the folder on PreCreate and PreApply would it be safe to ignore node_modules in SymlinkValidator

If so would that also be true for any other folders that we exclude? For instance we exclude the files directories in SiteFilesExcluder for PreCreate and PreApply but if there are symlinks those directories we would still stop any operations.

Proposed resolution

Github Composer stager branch https://github.com/TravisCarden/composer-stager/tree/feature/preconditio...

Issue fork automatic_updates-3304365

Command icon 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:

About issue forks

Comments

tedbow created an issue. See original summary.

traviscarden’s picture

traviscarden commented

Composer Stager does not currently take exclusions into account in its precondition test for symlinks. (c.f. CodeBaseContainsNoSymlinks.php. It would require a small (BC-safe) API addition to add support. I can work on that if/whenever you want to prioritize it.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Issue summary: View changes
traviscarden’s picture

traviscarden commented
Assigned: Unassigned » traviscarden

I'm currently working on the aforementioned feature addition to Composer Stager.

traviscarden’s picture

traviscarden commented
Assigned: traviscarden » Unassigned

The required enhancement has been merged into Composer Stager and released (v1.2.0), and the module has been updated to require it in #3317409: Update Composer Stager to v1.2.0. This is ready to move forward.

phenaproxima made their first commit to this issue’s fork.

phenaproxima opened merge request !564

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Issue tags: +core-mvp
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review
Issue tags: +Needs manual testing

This is probably ready for review.

I'm not really sure how else we could test this because:

  • the ignoring of the paths is handled and tested by Composer Stager
  • vfsStream doesn't support symlinks
  • Our kernel tests don't deal with the real file system

So I think we probably should get this manually tested, and merge it.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs review » Needs work

Project Browser runs a status check to determine if UI installs are possible. If the status check fails then there's a warning and the UI does not make the install controls available.

With the changes here, the status check is still failing and preventing the UI install
It looks like it doesn't run on status check?

if ($event instanceof PreCreateEvent || $event instanceof PreApplyEvent) {
      $exclusions = new PathList($event->getExcludedPaths());
    }
    else {
      $exclusions = NULL;
    }

I made a temporary change to make StatusCheckEvent part of that condition + added use ExcludedPathsTrait; to the event and that did not address it. I'm guessing the fact that StatusCheckEvent isn't already taking excluded paths into account is due to complexities that make it more involved than just adding ExcludedPathsTrait

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Issue tags: -Needs manual testing

Manually tested with Project Browser, it's good!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Assigned: Unassigned » tedbow
Status: Needs work » Needs review

Crediting @bnjmnm for manual testing, and assigning to @tedbow for final review.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Needs work

Needs test coverage the for the fact that our stage actually get the collected events and passes them on

and `SymlinkValidatorTest` doesn't test any other events but `PreCreate`

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review

`SymlinkValidatorTest` doesn't test any other events but `PreCreate`

I'd like to handle this in another issue. It's a pre-existing problem, and therefore out-of-scope here. Besides, the structure of both the test and the validator make it quite hard to test pre-apply without significant refactoring.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Reviewed & tested by the community

Looks good!

  • phenaproxima committed 81d6ccb on 8.x-2.x 
    Issue #3304365 by phenaproxima, tedbow, TravisCarden, bnjmnm: Do not...
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Reviewed & tested by the community » Fixed

I'm really happy with the solution we landed on. Merrily merged into 8.x-2.x.

Status: Fixed » Closed (fixed)

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