Harden LockFileValidator, add stopPropagation() at failures

Looks like a solid piece of hardening!

Excellent find! 👏

I am not sure why this is needed. See MR comment

I'd like to flip the question around: when should you use stopPropagation()? When a validator encounters an irrecoverable, fundamental problem, right?

Currently these stop propagation:

1. \Drupal\package_manager\Validator\StageNotInActiveValidator
2. \Drupal\package_manager\Validator\ComposerJsonExistsValidator
3. \Drupal\package_manager\Validator\EnvironmentSupportValidator
4. (and this one for testing purposes: \Drupal\Tests\automatic_updates\Kernel\StatusCheck\StagedProjectsValidatorTest::testEventConsumesExceptionResults())

Isn't the lock file not being hashable in \Drupal\package_manager\Validator\LockFileValidator::storeHash() (which runs during PreCreateEvent) equally essential? Why should we even create a stage if the lock file cannot be hashed?

Same thing for \Drupal\package_manager\Validator\LockFileValidator::validateStagePreOperation() during PreRequireEvent.

AFAICT #5 further elevates the importance of 📌 Define the Package Manager API (package_manager.api.php is outdated) Fixed .

Note that AFAICT this was done because the GitExcluder ran into this at some point: it looks like this was introduced in response to https://git.drupalcode.org/project/automatic_updates/-/merge_requests/54....

But I see that you created 📌 Update Package Manager event documentation in package_manager.api.php Fixed in part to address my general question of why/when one should use ::stopPropagation()! 🤩👍

Thanks for the link in #9

I see that https://git.drupalcode.org/project/automatic_updates/-/merge_requests/54...

I don't think the assumption in that comment was never correct.

Lets Consider package_manager/tests/src/Kernel/LockFileValidatorTest.php/testCreateWithNoLock() where it expects error on PreCreate and fails there so the actual stage is never destroyed and the we call assertStatusCheckResults which tries to get ignored paths and since we are already past precreate but errored out earlier we probably haven't claimed the stage or destroyed it either so GitExcluder assumes a stage should exist which doesn't

this was because of previous version of `GitExcluder`(not the version that got committed) was trying to keep state and assumed that if PreCreate was fired there would be a stage directory. That was incorrect there is always the possibility that any validator could add an error and therefore the stage directory could not exist. PreCreate being fired does not mean a stage would exist.

I think LockFileValidator was used an example but it literally could have been any validator.

So that original issue never need to do
protected $disableValidators = ['package_manager.git_excluder'];
in the first place as far as I can tell it should have been just checking for the existing the stage directory. but it didn't the logic that needed that anyways.

Setting to needs work for either an issue summary explaining why we need this issue at all or to close the issue.

I think in general we should never create an issue with completely empty summary because insight into why it is needed. Unless it is something like "$var is never used"

I do agree that the assumption in GitExcluder was wrong and yeah the early stop propagation would not allow for better messages +1 closing issue, assigning to @wim.leers for final comment.

Re-read everything. Sounds good. Especially because https://git.drupalcode.org/project/automatic_updates/-/merge_requests/54... did not end up adding a protected $disableValidators = […]; at all 👍