GitExcluder should not ignore .git directories that belong to packages installed by Composer

We need to update the tests.

Right we test 2 .git directories are ignore we need to test that .git won't be ignore if we find a a matching install_path

@kunal.sachdev thanks. I think this is close. The realpath + VFS problem is tricky

A couple changes needed and a small change to how to test realpath() based on our scrum discussion.

Pretty close though

@kunal.sachdev thanks looks good. I made 1 comment change. will merge when green

I think this looks pretty good and I agree with @tedbow's RTBC, but I'd like to make a few phrasing changes (and comment additions) to make sure this is well-documented. Self-assigning to handle that.

I think there may be another flaw here -- what if, in the stage directory, a package is added which is installed from source? We should be sure we're not excluding that from being copied back into the active directory.

@phenaproxima, can you please confirm that in #15 you actually wanted to see this test coverage: https://git.drupalcode.org/project/automatic_updates/-/merge_requests/54... ? 🙏

Alright, yay, this is actionable again!

Need some clarity on what exactly indirect dependencies mean are they just dependencies of dependencies ? and where do these indirect dependencies live exactly just as other contrib modules?

Need some clarity on what exactly indirect dependencies mean are they just dependencies of dependencies ? and where do these indirect dependencies live exactly just as other contrib modules?

See #15 — it's referring to a composer package that was installed because another composer package was installed.

For example: you want to install foo/foo. But foo/foo depends on foo/core. ⇒ We should verify that foo/core is also processed correctly.

1. Right now we can only test for the case when a new module(fetched from source) is added in the stage and should be reflected in the active.
2. The case where we remove a package in stage cannot be tested due do the limitations of \Drupal\package_manager_bypass\Committer since it does not respect the exculsions sent from \Drupal\package_manager\PathExcluder\GitExcluder and copies everything from stage to active directory.
3. The other test \Drupal\Tests\package_manager\Kernel\PathExcluder\GitExcluderTest::testGitDirectoriesExcludedActive works as expected and the validator seems to work as intended while copying directories from active to stage directory.

Not asserting using invocation args in testGitDirectoriesExcludedStage as there won't be any reliable assertion for a new package being added in the stage.

Created #3328600: Update a build test for integration testing and be able to verify directory changes in stage. for build test change.

12 remarks on the MR, a few questions for @tedbow, but mostly clean-up that's necessary before I can RTBC this for @tedbow to review 🤓

I did a very thorough review of this issue that keeps surprising us! 🤓

14 remarks. Some of them nits about formatting/naming/etc. But also some genuine code clarity/maintainability remarks, and a few fairly fundamental questions.

So close!

Postponed on #3330139: Harden PathExclusionsTrait::excludeInProjectRoot() for paths outside of project root.

Created #3330140: Update StatusCheckTrait::runStatusCheck() to reorder/avoid dispatching CollectIgnoredPathsEvent earlier than StatusCheckEvent

#3330139: Harden PathExclusionsTrait::excludeInProjectRoot() for paths outside of project root. fixed

Yep — adjusting title 🤓

Revisiting #3330140: Update StatusCheckTrait::runStatusCheck() to reorder/avoid dispatching CollectIgnoredPathsEvent earlier than StatusCheckEvent I think it needs to happen first as we will always hit the excluder first and don't have proper validation for a missing composer file/(an issue while computing ignored_paths) in it.

Reflecting #39.

#3330140: Update StatusCheckTrait::runStatusCheck() to reorder/avoid dispatching CollectIgnoredPathsEvent earlier than StatusCheckEvent fixed

I know I said we could just include the case we missed in #3330140: Update StatusCheckTrait::runStatusCheck() to reorder/avoid dispatching CollectIgnoredPathsEvent earlier than StatusCheckEvent in this MR. But I noticed there's another case. So we'll need to handle that first.

Please move https://git.drupalcode.org/project/automatic_updates/-/merge_requests/54... into a separate MR and handle + test the extra case there. That can then land swiftly.

It'll keep the scope here simpler.

To avoid such a late discovery in the future, I suggest testing the stack-of-MRs (or patches) locally to verify that tests are passing. Or just create a patch (or MR) that represents the full stack of changes, then we'd have known days ago that not all cases were handled in #3330140!

Just testing if this passes and then moving validation and tests to a separate MR.

created #3334054: Add error handling for \Drupal\package_manager\Stage::getIgnoredPaths() and postponed on it.

So the thing is we can't really test our changes in #3334054: Add error handling for \Drupal\package_manager\Stage::getIgnoredPaths() because we don't have an excluder that fails on a call to the composer, basically we need the new git excluder for that so I think we should close that issue and add the validation in this issue itself. (The validation and test for it is already pushed up in the MR of the current issue.)

#46: that seems reasonable! Thank you for giving it a try! 😊🙏

@tedbow: see #3334054-4: Add error handling for \Drupal\package_manager\Stage::getIgnoredPaths().

Also, I have one more significant concern, for which I only just now discovered the potentially superior way to do it, thanks to working on #3331168: Limit trusted Composer plugins to a known list, allow user to add more. I'll let @tedbow decide though. @tedbow, see the MR, the mention of \Composer\Package\Package::getDistType().

I pushed up a commit to go with my MR comment but it will still need tests.

@omkar.podey thanks for adding the new test but I think there is problem

I checked about the previous version of package_manager/src/PathExcluder/GitExcluder.php before my change

and ran your new test \Drupal\Tests\package_manager\Kernel\PathExcluder\GitExcluderTest::testGitDirectoriesExcludedIsDir and it still passed.

so this either means

1. I missed something and problem I mentioned with scanStage was actually not problem
2. or the new test doesn't test the situation I was describing

I haven't looked into enough to know yet.

I just discovered this can also happen even when you're using --prefer-dist: when you specify a version that can only be available through VCS such as dev-master: https://github.com/composer/composer/issues/6370.

@tedbow I'm especially curious about your thoughts on my lengthy research comment at https://git.drupalcode.org/project/automatic_updates/-/merge_requests/54... — IMHO we should open a Normal Postponed Task tagged core-post-mvp titled Research removing ComposerUtility::getInstalledPackagesData() because it does not use public composer APIs. I'll do that if you agree after reading my comment 🤓

I also thought about the situation when (which might be obvious for everyone)

1. A package fetched from source in active is copied to stage.
2. Then the package gets deleted in stage.
3. Should not be copied back to active.
4. Which will be taken care of by the composer-stager's rsync file copy as anything not in stage will be deleted from active while copying.

I reopened #3334054: Add error handling for \Drupal\package_manager\Stage::getIgnoredPaths() and copied over whatever can be independently committed (without removing stuff from this MR for now), i still need to look at fixtures, But some tests related to the error handling of \Drupal\package_manager\Stage::getIgnoredPaths have to be merged with the current MR.

1. The fixture files being added in the issue are required as they have _git folders.
2. The only common test change between this issue and #3334054: Add error handling for \Drupal\package_manager\Stage::getIgnoredPaths() is in package_manager/tests/src/Kernel/ComposerJsonExistsValidatorTest.php

Thanks for #59! 👍 That did definitely pre-emptively answer a few review questions 😄 Confirmed that reverting.

5 remarks on the MR, all around clarity. But this is definitely in the very final stretch! 👏

Manually did phpcs locally, all green 👍

The follow-up I requested in #56 was created: #3334994: Add new InstalledPackagesList which does not rely on Composer API to get package info.

Needs work

See question I put on the MR about the need for the new fixture

We just put a lot of effort into getting rid of most fixture folders so I want make sure we need this one.

I agree, we can definitely create addDotGitFolder

created #3335802: Add addDotGitFolder functionality to \Drupal\fixture_manipulator\FixtureManipulator

I like it! 😄

Postponing on #3335802: Add addDotGitFolder functionality to \Drupal\fixture_manipulator\FixtureManipulator.

This blocks #3334552: Ensure that intentionally failed test stage directories are deleted after tests complete, so tagging.

If the test above passes it proves the compatibility with #3335802: Add addDotGitFolder functionality to \Drupal\fixture_manipulator\FixtureManipulator

fixed #3335802: Add addDotGitFolder functionality to \Drupal\fixture_manipulator\FixtureManipulator

I'll get this across the finish line while @omkar.podey is on PTO.

First, git merge origin/8.x-2.x…

Since #71:

1. removed the fixture that was possible to remove thanks to #3335802: Add addDotGitFolder functionality to \Drupal\fixture_manipulator\FixtureManipulator
2. and then reviewed + minimized the diff:
   1. fixed an incorrectly resolved merge conflict (by deleting code 😁)
   2. reverted the LockFileValidator ::stopPropagation() additions which seem to be obsolete now.
   3. reverted the StagedDBUpdateValidatorTest change because that seems to be obsolete too now

That took this MR down from 13 changed files to a mere 3!

phpcs passes locally BTW 👍

Thanks everyone! A lot of work but a big win we don't message with git folder managed by Composer!