Create an API for base requirement validators which run before other validators and stop event propagation if they fail

📌 Update Package Manager event documentation in package_manager.api.php Fixed documented when this is appropriate to use, and which ones use it today.

Over at #3342430-18: [PP-1] Hard failure after module install if composer is not found → , @kunal.sachdev and I discovered that this issue is quite literally what the next step over there was going to be. Therefore this issue is now hard-blocking that one.

Elevating from core-post-mvp to core-mvp.

The core-post-mvp tag did make sense when this issue was created ~4 months ago: we did not yet have ComposerInspector, we were basically nowhere yet with 📌 Remove our runtime dependency on composer/composer: remove ComposerUtility Fixed , so the presence of the composer command was not equally critical as it is today!

yash.rode → made their first commit to this issue’s fork.

kunal.sachdev → made their first commit to this issue’s fork.

The only thing missing: updating package_manager.api.php (hence ).

Wim Leers → closed merge request !513

🚀

This definitely fixes the problem and adds the proper test coverage...but the logic is becoming confusing to read. I have suggested a refactoring. (We could do it in a follow-up, if you'd prefer, but then please open that issue before we commit this.)

Ok, I can do that refactoring in this issue only.

Per @tedbow's 📌 Add a validate() method to ComposerInspector to ensure that Composer is usable Fixed , bumping to .

I still don't generally agree with this issue and I think we should close it. My comment from the previous MR

this would only fail horribly if someone used '@PhpTuf\ComposerStager\Domain\Service\ProcessRunner\ComposerRunnerInterface' service correct? otherwise if they used php classes provided by composer/composer they should be fine.
Since generally Composer stager is wrapper around composer I don't think there would be many cases of invoking the composer executable in a listener.
I just generally don't want to hide other problems with system unless we really have to for instance.
if we were to do this change you might only see the Composer version problem and think "great I just have to fix the composer version and then I would be good to go" but you might say miss the Writable file system validator which may actually be more important because it might unsolvable for you hosting. So may be waste of time to fix the composer version.

@Wim Leers replied yesterday

This was true ~4 months ago, but not anymore today, because of ComposerInspector. That is used ubiquitously, and calls composer, and hence the absence of composer should stop propagation: otherwise 99% of the codebase will have to repeat similar checks!

I still think by comment about hiding problems applies. We don't know what people are going to be writing validators for. If we exit early we are giving the user less context.

For example someone may write a validator that checks something specific and very basic about there hosting and reports "Hosting problem X please fix before you can use package manager", but really this could be any validator

If they don't know to make sure their validator runs before ours then I think this totally could be a likely outcome

1. site admin tries to use Automatic updates the admin user only sees "A Composer version which satisfies...."
2. The drupal admin user doesn't have access or doesn't know how to update Composer on the hosting so emails Server Admin Person internally or at hosting company to update Composer
3. Because Server Admin Person is busy it takes a couple of days for them to update Composer
4. Server Admin Person emails back, "composer is updated you should be ok"
5. The admin tries to use Automatic Update again but only now sees "Hosting problem X please fix before you can use package manager" or any other validator message were hiding from them.
6. The drupal admin user doesn't have access or doesn't know how to solve "Hosting problem X" so they again email Server Admin Person
7. Because Server Admin Person is busy it takes a couple of days for them fix "Hosting problem X"
8. Server Admin Person emails back, "Hosting problem X is fix you should be ok"
9. The admin tries to use Automatic Update again everything is great

Or it could be

1. site admin tries to use Automatic updates the admin user only see "A Composer version which satisfies...." and "Hosting problem X please fix before you can use package manager"
2. The admin tries to use Automatic Update again but only now sees "Hosting problem X please fix before you can use package manager" or any
3. The drupal admin user doesn't have access or doesn't know how to update Composer on the hosting or fix "Hosting problem X"so Server Admin Person internally or at hosting company to fix both things
4. Because Server Admin Person is busy it takes a couple of days for them fix both problems
5. Server Admin Person emails back, "Hosting problem X is fixed and Composer isupdate, you should be ok"
6. The admin tries to use Automatic Update again everything is great

I think this examples shows that we don't really want to hiding info from the user. I think it is very likely that people that are not using UI's based on Package Manager will not know how or have permission to solve all the server related problems and they should have as much context as possible to pass onto other people.
The 2nd problem above could also have been from StageNotInActiveValidator and would have the same problem
Also even if the person who is using the UI knows how to solve the problems it could the 2 problems are interrelated so they might be better able to solve them if they had more context.

otherwise 99% of the codebase will have to repeat similar checks!
I don't think 99% of the codebase is validators that use Composer inspector but even for the ones that are this easily solvable, in 📌 Add a validate() method to ComposerInspector to ensure that Composer is usable Fixed with something like this

trait ComposerDependentValidatorTrait {

  protected ComposerInspector $composerInspector;

  public function validate(StageEvent $event) {
    try {
      $this->composerInspector->validate();
    }
    catch (\Throwable $exception) {
      $this->handleInvalidComposer($exception);
      return;
    }
    $this->composerValidated($event);
  }

  protected function handleInvalidComposer(\Throwable $exception) {
    // Can override to give a specific message.
    return;
  }

  abstract protected function composerValidated(StageEvent $event): void;
  
}

or a ComposerDependentValidatorBase

Lack of issue triage causing wasted work

I still don't generally agree with this issue and I think we should close it.

😳🤯 Then why is it open/around? Why was it tagged core-post-mvp and not closed?

That level of doubt was not reflected anywhere on this issue. What you're repeating now is from a MR comment from October, almost 5 months ago 😬 Too bad @kunal.sachdev was working on this for no good reason then. 😞 If the tech lead thinks something is not ready to work on or probably should be closed, it should be very explicitly reflected. For example, by marking it .

Ted's analysis in #24

For example someone may write a validator that checks something specific and very basic about there hosting and reports "Hosting problem X please fix before you can use package manager", but really this could be any validator

Great example!

I still think by comment about hiding problems applies. We don't know what people are going to be writing validators for. If we exit early we are giving the user less context.

Right. But that statement implies that the current architecture for validators is wrong. It appears that the current architecture (using event subscribers with no additional infrastructure) is insufficient to solve the "package manager validation" problem space.

Currently these validators call ::stopPropagation():

1. \Drupal\package_manager\Validator\StageNotInActiveValidator
2. \Drupal\package_manager\Validator\EnvironmentSupportValidator
3. \Drupal\package_manager\Validator\ComposerJsonExistsValidator

(And we recently documented that in 📌 Update Package Manager event documentation in package_manager.api.php Fixed .)

But based on what you're saying right now, those 3 are also problematic, because actually we should be able to see the results for all three at the same time, and right now the first one wins. Each of them handles a different aspect of package manager validation:

1. file system layout
2. execution environment
3. composer metadata

I would classify ComposerExecutableValidator as also falling under that "execution environment" aspect.

My analysis building on Ted's

So ComposerDependentValidatorBase would be one way forward 👍 But it'd still only solve a subset of the problem.

What we need is something similar to \PhpTuf\ComposerStager\Domain\Aggregate\PreconditionsTree\PreconditionsTreeInterface, where \PhpTuf\ComposerStager\Infrastructure\Aggregate\PreconditionsTree\AbstractPreconditionsTree::assertIsFulfilled() effectively iterates over all the "child preconditions" in the order they're assigned, and it stops evaluating after the first one. The difference is that we would want to execute them all, except if that particular "aspect" has a fundamental requirement without which all the other validation no longer makes sense.

But the question is if it is even possible to define those aspects in a way that A) there is no overlap, B) there is a clear "fundamental precondition" one for each. For "execution environment" for example, it could be: operating system, composer version, Xdebug on/off, etc. So … I tried:

… and I don't see how that'd allow us to solve the entire problem after all 🙈😬

Conclusion

I can only conclude that Ted is right 😊, and that we should:

1. StageNotInActiveValidator should never have been stopping propagation either?
2. EnvironmentSupportValidator is a misnomer; it's not really a validator. It's really a very low-level thing that a hosting provider can specify to prevent the package_manager from being used on their infrastructure altogether. In fact, arguably this ought to be moved to hook_requirements()? That won't catch the case of the hosting provider specifying it after the module was installed. For that edge case, I think we ought to treat the presence of this environment variable as another failure marker, for which we have FailureMarker (which landed after this validator).
3. for ComposerJsonExistsValidator + ComposerExecutableValidator, what you propose at the end of the issue summary at 📌 Add a validate() method to ComposerInspector to ensure that Composer is usable Fixed makes sense, specifically:
   

   Meanwhile, let's remove ComposerJsonExistsValidator and ComposerExecutableValidator, and fold them into a new validator: ComposerValidator, which simply calls ComposerInspector::validate() and bubbles any exception it raises up into a validation error. ComposerValidator should have a high priority (10000) so it runs first, and stops propagation if an error is detected. Something like:

👆 Then we solve the full problem space!

IMHO 📌 Add a validate() method to ComposerInspector to ensure that Composer is usable Fixed should be closed and folded into this issue, because this issue A) predates that one, B) has a lot more discussion, C) solves the entire problem, not a subset.

Attached is a starting point that implements the first 2 points (because those were not yet proposed by @tedbow so I wanted to check viability) — implementing ComposerValidator and ComposerDependentValidatorBase is still TBD.

Explanations on the PoC patch I posted for context:

1. +++ b/package_manager/package_manager.api.php
   @@ -102,17 +102,6 @@
   - *  at all, or a security vulnerability is detected, the event propagation must
   

   👋 Bye bye complexity.

2. +++ b/package_manager/src/FailureMarker.php
   @@ -17,6 +20,10 @@ use Drupal\package_manager\Exception\ApplyFailedException;
   + * Alternatively, the presence of a special environment variable is also treated
   + * as a failure marker: it allows hosting companies to prevent Package Manager
   + * from being used on their infrastructure.
   

   This is the fundamental reasoning.

3. +++ b/package_manager/src/FailureMarker.php
   @@ -74,6 +94,7 @@ final class FailureMarker {
      public function assertNotExists(): void {
   +    $this->validateEnvironment();
   

   Before checking the failure marker file, we check that other failure marker: an environment that disallows us!

   Note that Stage already calls this long before even beginning to trigger events, which is exactly why I believe this is a far better place to run such a fundamental check.

   (We can bikeshed the name FailureMarker, but that's besides the point for now. Especially because it's @internal.)

4. +++ b/package_manager/src/FailureMarker.php
   @@ -89,4 +110,29 @@ final class FailureMarker {
   +    throw new ApplyFailedException((string) $message);
   

   I know that this is very much a misnomer. But it already is. (When the failure marker is pre-existing when attempting to create a stage.)

   And 📌 Refactor exception architecture Fixed is already fixing that! 👍

5. +++ b/package_manager/src/Validator/ComposerJsonExistsValidator.php
   @@ -37,8 +37,6 @@ final class ComposerJsonExistsValidator implements EventSubscriberInterface {
   -    // Set priority to 190 which puts it just after EnvironmentSupportValidator.
   -    // @see \Drupal\package_manager\Validator\EnvironmentSupportValidator
   

   👋 Bye bye complexity.

6. +++ b/package_manager/src/Validator/ComposerJsonExistsValidator.php
   @@ -56,6 +54,7 @@ final class ComposerJsonExistsValidator implements EventSubscriberInterface {
   +      // ⚠️ This is the ONLY validator that stops propagation!
   

   🚨 The only place allowed to do this!

7. +++ b/package_manager/tests/src/Kernel/EnvironmentSupportValidatorTest.php
   @@ -14,6 +14,7 @@ use Drupal\package_manager\Validator\EnvironmentSupportValidator;
   + * @requires PHP < 8.0
     */
    class EnvironmentSupportValidatorTest extends PackageManagerKernelTestBase {
   

   This bit didn't make it into the patch:

   @todo MOVE THIS TEST COVERAGE INTO FailureMarkerRequirementTest AND FailureMarkerTest
   

   (This was just a trick to not run this test anymore.)

For that edge case, I think we ought to treat the presence of this environment variable as another failure marker, for which we have FailureMarker (which landed after this validator).

I agree that, if we expand what FailureMarker checks for it, it is no longer correctly named.

But we do need to bear in mind that FailureMarker was originally written in order to detect a CATASTROPHIC failure during the stage lifecycle. The kind of failure that makes the code base non-viable and needing to be restored from backup.

Does a kill-switch environment variable really fall into that category?

Just something to think about as we change this...

1. That level of doubt was not reflected anywhere on this issue. What you're repeating now is from a MR comment from October, almost 5 months ago 😬 Too bad @kunal.sachdev was working on this for no good reason then. 😞

   After I made that comment no one responded. I then marked the issue core-post-mvp, I should have given more context but that reflects I do not think we have to solve this problem before we go into core and every core-mvp is more important.

2. EnvironmentSupportValidator is a misnomer; it's not really a validator. It's really a very low-level thing that a hosting provider can specify to prevent the package_manager from being used on their infrastructure altogether. In fact, arguably this ought to be moved to hook_requirements()?

   Not sure which $phase you meant for hook_requirements. Assuming you mean install.

   We considered that in #3109082: Allow hosting platforms to declare that they don't support Package Manager → where we put in this validator and then split off into #3313507: Don't allow Automatic Updates to be installed if the platform doesn't support Package Manager → which we decide not to do.

   We opted that we should at least have #3306283: If cron updates are disabled display a message if status checks fail after installing Automatic Updates → so you are always notified if you install AutoUpdates if there are any problems so you know right away since you are likely relying on this for critical security updates

3. For that edge case, I think we ought to treat the presence of this environment variable as another failure marker, for which we have FailureMarker (which landed after this validator).

   This is not a failure of an operation so I think this belongs in here

4. I think EnvironmentSupportValidator is fine as a validator. It is a special case validator that should stop propagation because there is not other error you could receive that would fix your environment simply being flagged as not supporting package manager.

   I think we can remove stopPropagation() from all other validators.

   I think we should document that you should not propagation unless the error indicates there no possible solution to getting package manager to work on your system, as EnvironmentSupportValidator does. Every other validator that we have is problem that might be able to be fixed. Even WritableFileSystemValidator could be fixed and if not the env var should set for EnvironmentSupportValidator to pickup.

I am going to continue working on 📌 Add a validate() method to ComposerInspector to ensure that Composer is usable Fixed we are not on agreement on what should happen here and that issue can proceed.

Personally I think we should

1. remove stopPropagation() everywhere except EnvironmentSupportValidator
2. Update the docs to reflect that it should only be used if the issue unsolvable. But generally if this is something that is server level restriction they should be using EnvironmentSupportValidator which provides a way for you to give the user a link for more info.

re #25

Lack of issue triage causing wasted work

just be clear, since there was confusion before and #28 was maybe rambling Do not work any more on MR of this issue until we get consensus

I am just not postponing it because we need more discussion

@phenaproxima is working on 📌 Add a validate() method to ComposerInspector to ensure that Composer is usable Fixed

#27: agreed they're different semantically. But they are similar technically. They have the same kind of requirements in terms of when they run, and that they trump all other considerations.

Suggested name: KillswitchPreconditions, with FailureMarker being one killswitch and HostingEnvironmentOptOut another.

#28:

1. Yes, $phase === 'install'. I see you wrote there:
   

   So this would mean that Project Browser and other modules like it that have functionality even if the current environment does not support Package Manager can't be enabled in an environment unless they do not add Package Manager as a dependency. So they would have to prompt users to enable Package separately. That doesn't seem like the best UX for Project Browser users that do have an environment that supports Package Manager because it adds an extra step.

   ⇒ Fair enough!

   Then why not go with $phase === 'runtime'? That means you can install the module but will immediately be told.

2. See my response to #27.
3. Hm … now that you put it that way, it makes a ton of sense! 😄

RE your 2-point proposal at the end: I think you mean to keep stopPropagation() in 2 places: EnvironmentSupportValidator and the upcoming ComposerValidator?

#29 👍👍👍

I have pushed up new MR on the 3312960-base-requirements-validators branch with another proposal

The basic idea with this MR is that there are "base requirements" validators which need be run before most other validators can be run. This are validators that other validators might rely on such as Composer validation or validate something basic about the system like a writable file system.

These "base requirements" validators don't stop propagation themselves because we want all of them to able run but this MR introduces a new BaseRequirementsFulfilledValidator is run after all of the base requirements validators which if any errors have been flagged will stop propagation itself.

There is a new BaseRequirementsFulfilledValidator::PRIORITY constant to make it easy for all "base requirements" validators to run before the BaseRequirementsFulfilledValidator and a BaseRequirementValidatorTrait that most of these validators could use.

This idea is sort of a comprise where we acknowledge that some validators are more fundamental than others and most validators should not be run if these file while still letting all in this group run so the user gets as much context as possible by not having any of these individual validators stop propagation.

#33: sounds good, that achieves the same as what I wrote at the end of #25 👍

But 📌 Add a validate() method to ComposerInspector to ensure that Composer is usable Fixed caused the MR to no longer apply. Updated it.

But I see that @phenaproxima opened another MR/branch, named just-remove-it.

Can you explain what you didn't like about @tedbow's proposal, @phenaproxima? Or what you were exploring there?

phenaproxima → closed merge request !732

But I see that @phenaproxima opened another MR/branch, named just-remove-it.

Can you explain what you didn't like about @tedbow's proposal, @phenaproxima? Or what you were exploring there?

If I remember correctly, I was exploring essentially what we ended up committing in 📌 Add a validate() method to ComposerInspector to ensure that Composer is usable Fixed : we don't need separate validators for the existence of composer.json and the version/presence of the Composer executable. We can have ComposerInspector do it all. I was trying to see how many tests would break.

I've closed that MR now.

AFAICT @tedbow's MR731 is the direction that @tedbow and @phenaproxima proposed for this issue. I've already reviewed it — and @tedbow has already answered the initial questions I had.

AFAICT that means that is no longer true? 😄🥳

@tedbow, do you intend to finish this?

I'm fine with the direction of !731. If we're all on board, then I think we should finish it.

👍 Then if we all are — can either you or @tedbow drive it to the finish line? I'm working on composer-stager + 📌 Remove dependency on symfony/finder Fixed .

The current test failure is in \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::testApi because we call \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::assertReadOnlyFileSystemError which ensures that the update is prevented if the web root and/or vendor directories are not writable but now \Drupal\package_manager\Validator\WritableFileSystemValidator stops propagation if any error is there and hence in our tests it stops propagation for PreCreateEvent. But in test we assert it’s fired multiple times where in actual it's fired only one time now.

Though there is a comment

// ::assertReadOnlyFileSystemError attempts to start an update
        // multiple times so 'PreCreateEvent' will be fired multiple times.
        // @see \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::assertReadOnlyFileSystemError()

to explain why it was being fired multiple times before this, but I couldn't figure out how ::assertReadOnlyFileSystemError attempts to start update multiple times.

#41: Did the "dump extra info into dblog and print the dblog HTML to a file so you can inspect it later" strategy that I proposed not provide the necessary extra info? 😢

I tried that strategy and I get the backtrace Array ( [0] => Array ( [function] => logEventInfo [class] => Drupal\package_manager_test_event_logger\EventSubscriber\EventLogSubscriber [type] => -> ) [1] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php [line] => 108 [function] => call_user_func ) [2] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/modules/contrib/automatic_updates/package_manager/src/Stage.php [line] => 523 [function] => dispatch [class] => Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher [type] => -> ) [3] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/modules/contrib/automatic_updates/src/Updater.php [line] => 104 [function] => dispatch [class] => Drupal\package_manager\Stage [type] => -> ) [4] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/modules/contrib/automatic_updates/package_manager/src/Stage.php [line] => 300 [function] => dispatch [class] => Drupal\automatic_updates\Updater [type] => -> ) [5] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/modules/contrib/automatic_updates/src/Updater.php [line] => 65 [function] => create [class] => Drupal\package_manager\Stage [type] => -> ) [6] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/modules/contrib/automatic_updates/tests/modules/automatic_updates_test_api/src/ApiController.php [line] => 32 [function] => begin [class] => Drupal\automatic_updates\Updater [type] => -> ) [7] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/modules/contrib/automatic_updates/package_manager/tests/modules/package_manager_test_api/src/ApiController.php [line] => 96 [function] => createAndApplyStage [class] => Drupal\automatic_updates_test_api\ApiController [type] => -> ) [8] => Array ( [function] => run [class] => Drupal\package_manager_test_api\ApiController [type] => -> ) [9] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php [line] => 123 [function] => call_user_func_array ) [10] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/core/lib/Drupal/Core/Render/Renderer.php [line] => 580 [function] => Drupal\Core\EventSubscriber\{closure} [class] => Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber [type] => -> ) [11] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php [line] => 124 [function] => executeInRenderContext [class] => Drupal\Core\Render\Renderer [type] => -> ) [12] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php [line] => 97 [function] => wrapControllerExecutionInRenderContext [class] => Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber [type] => -> ) [13] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/vendor/symfony/http-kernel/HttpKernel.php [line] => 163 [function] => Drupal\Core\EventSubscriber\{closure} [class] => Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber [type] => -> ) [14] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/vendor/symfony/http-kernel/HttpKernel.php [line] => 74 [function] => handleRaw [class] => Symfony\Component\HttpKernel\HttpKernel [type] => -> ) [15] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/core/lib/Drupal/Core/StackMiddleware/Session.php [line] => 58 [function] => handle [class] => Symfony\Component\HttpKernel\HttpKernel [type] => -> ) [16] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php [line] => 48 [function] => handle [class] => Drupal\Core\StackMiddleware\Session [type] => -> ) [17] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/core/modules/page_cache/src/StackMiddleware/PageCache.php [line] => 106 [function] => handle [class] => Drupal\Core\StackMiddleware\KernelPreHandle [type] => -> ) [18] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/core/modules/page_cache/src/StackMiddleware/PageCache.php [line] => 85 [function] => pass [class] => Drupal\page_cache\StackMiddleware\PageCache [type] => -> ) [19] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php [line] => 48 [function] => handle [class] => Drupal\page_cache\StackMiddleware\PageCache [type] => -> ) [20] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php [line] => 51 [function] => handle [class] => Drupal\Core\StackMiddleware\ReverseProxyMiddleware [type] => -> ) [21] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/core/lib/Drupal/Core/StackMiddleware/StackedHttpKernel.php [line] => 51 [function] => handle [class] => Drupal\Core\StackMiddleware\NegotiationMiddleware [type] => -> ) [22] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/core/lib/Drupal/Core/DrupalKernel.php [line] => 675 [function] => handle [class] => Drupal\Core\StackMiddleware\StackedHttpKernel [type] => -> ) [23] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/index.php [line] => 19 [function] => handle [class] => Drupal\Core\DrupalKernel [type] => -> ) [24] => Array ( [file] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/.ht.router.php [line] => 65 [args] => Array ( [0] => /private/tmp/build_workspace_3718b3eb155e801f1a6132cd004267b3L6696b/project/web/index.php ) [function] => require ) ) for each PreCreateEvent, and from this I couldn't find anything that can explain it.

The end is in sight! 😄 2 remarks on the MR 😊

Wim Leers → closed merge request !723

🐛 Missed something! Overnight, ComposerValidator was made a non-base requirement validator. 🐛

The issue summary is wildly outdated. Rather than rewriting it, just pointing to the conclusions we reached.

🚀

Found some things that should be cleaned up before we merge this. But overall it makes sense!

I think @kunal.sachdev can tackle the remaining feedback 😊

Looking good just a few MR comments/suggestions

Back to @tedbow for review.

Needs work for test coverage but otherwise looks good

Thanks everyone!

Thanks everyone!

So great to have this in! 😊 The end result is clearly a significant improvement! 👏

I see that @kunal.sachdev beat me by one minute 🥺 to unpostpone 🐛 Hard failure after module install if composer is not found Needs review , which is now unblocked! 🥳

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