| Comment | File | Size | Author |
|---|---|---|---|
| #25 | 3312960-25-PoC.patch | 10.08 KB | wim leers |
| #25 | Screen Shot 2023-02-23 at 6.38.37 PM.png | 188.27 KB | wim leers |
Issue fork automatic_updates-3312960
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:
Comments
Comment #2
phenaproximaComment #3
phenaproximaComment #5
phenaproximaComment #6
phenaproximaAssigning to @tedbow for review.
Comment #7
tedbowComment #8
tedbowComment #9
tedbowComment #10
tedbowComment #11
wim leers#3336243: Update Package Manager event documentation in package_manager.api.php documented when this is appropriate to use, and which ones use it today.
Comment #12
wim leersOver at #3342430-18: 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-mvptocore-mvp.The
core-post-mvptag did make sense when this issue was created ~4 months ago: we did not yet haveComposerInspector, we were basically nowhere yet with #3316368: Remove our runtime dependency on composer/composer: remove ComposerUtility, so the presence of thecomposercommand was not equally critical as it is today!Comment #16
kunal.sachdev commentedComment #17
wim leersThe only thing missing: updating
package_manager.api.php(hence ).Comment #19
kunal.sachdev commentedComment #20
wim leers🚀
Comment #21
phenaproximaThis 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.)
Comment #22
kunal.sachdev commentedOk, I can do that refactoring in this issue only.
Comment #23
wim leersPer @tedbow's #3344039: Add a validate() method to ComposerInspector to ensure that Composer is usable, bumping to .
Comment #24
tedbowI still don't generally agree with this issue and I think we should close it. My comment from the previous MR
@Wim Leers replied yesterday
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, of example even our
StageNotInActiveValidatorIf they don't know to make sure their validator runs before ours then I think this totally could be a likely outcome
Or it could be
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 #3344039: Add a validate() method to ComposerInspector to ensure that Composer is usable with something like this
or a
ComposerDependentValidatorBaseAll validators that use this trait would have to do this subscribe event to
validateand implementcomposerValidatedComment #25
wim leersLack of issue triage causing wasted work
😳🤯 Then why is it open/around? Why was it tagged
core-post-mvpand 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
Great example!
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():\Drupal\package_manager\Validator\StageNotInActiveValidator\Drupal\package_manager\Validator\EnvironmentSupportValidator\Drupal\package_manager\Validator\ComposerJsonExistsValidator(And we recently documented that in #3336243: Update Package Manager event documentation in package_manager.api.php.)
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:
I would classify
ComposerExecutableValidatoras also falling under that "execution environment" aspect.My analysis building on Ted's
So
ComposerDependentValidatorBasewould 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:
StageNotInActiveValidatorshould never have been stopping propagation either?EnvironmentSupportValidatoris a misnomer; it's not really a validator. It's really a very low-level thing that a hosting provider can specify to prevent thepackage_managerfrom being used on their infrastructure altogether. In fact, arguably this ought to be moved tohook_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 haveFailureMarker(which landed after this validator).ComposerJsonExistsValidator+ComposerExecutableValidator, what you propose at the end of the issue summary at #3344039: Add a validate() method to ComposerInspector to ensure that Composer is usable makes sense, specifically:👆 Then we solve the full problem space!
IMHO #3344039: Add a validate() method to ComposerInspector to ensure that Composer is usable 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
ComposerValidatorandComposerDependentValidatorBaseis still TBD.Comment #26
wim leersExplanations on the PoC patch I posted for context:
👋 Bye bye complexity.
This is the fundamental reasoning.
Before checking the failure marker file, we check that other failure marker: an environment that disallows us!
Note that
Stagealready 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.)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 #3331355: Refactor exception architecture is already fixing that! 👍
👋 Bye bye complexity.
🚨 The only place allowed to do this!
This bit didn't make it into the patch:
(This was just a trick to not run this test anymore.)
Comment #27
phenaproximaI agree that, if we expand what FailureMarker checks for, 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...
Comment #28
tedbowAfter 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 everycore-mvpis more important.Not sure which
$phaseyou meant forhook_requirements. Assuming you meaninstall.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
This is not a failure of an operation so I think this belongs in here
EnvironmentSupportValidatoris 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
EnvironmentSupportValidatordoes. Every other validator that we have is problem that might be able to be fixed. EvenWritableFileSystemValidatorcould be fixed and if not the env var should set forEnvironmentSupportValidatorto pickup.I am going to continue working on #3344039: Add a validate() method to ComposerInspector to ensure that Composer is usable we are not on agreement on what should happen here and that issue can proceed.
Personally I think we should
Comment #29
tedbowre #25
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 #3344039: Add a validate() method to ComposerInspector to ensure that Composer is usable
Comment #30
wim leers#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, withFailureMarkerbeing one killswitch andHostingEnvironmentOptOutanother.#28:
$phase === 'install'. I see you wrote there:⇒ Fair enough!
Then why not go with
$phase === 'runtime'? That means you can install the module but will immediately be told.RE your 2-point proposal at the end: I think you mean to keep
stopPropagation()in 2 places:EnvironmentSupportValidatorand the upcomingComposerValidator?#29 👍👍👍
Comment #33
tedbowI 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
BaseRequirementsFulfilledValidatoris run after all of the base requirements validators which if any errors have been flagged will stop propagation itself.There is a new
BaseRequirementsFulfilledValidator::PRIORITYconstant to make it easy for all "base requirements" validators to run before theBaseRequirementsFulfilledValidatorand aBaseRequirementValidatorTraitthat 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.
Comment #34
wim leers#33: sounds good, that achieves the same as what I wrote at the end of #25 👍
But #3344039: Add a validate() method to ComposerInspector to ensure that Composer is usable 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?
Comment #36
phenaproximaIf I remember correctly, I was exploring essentially what we ended up committing in #3344039: Add a validate() method to ComposerInspector to ensure that Composer is usable: 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.
Comment #37
wim leersAFAICT @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?
Comment #38
phenaproximaI'm fine with the direction of !731. If we're all on board, then I think we should finish it.
Comment #39
wim leers👍 Then if we all are — can either you or @tedbow drive it to the finish line? I'm working on
composer-stager+ #3321933: Remove dependency on symfony/finder.Comment #40
kunal.sachdev commentedComment #41
kunal.sachdev commentedThe current test failure is in
\Drupal\Tests\automatic_updates\Build\CoreUpdateTest::testApibecause we call\Drupal\Tests\automatic_updates\Build\CoreUpdateTest::assertReadOnlyFileSystemErrorwhich ensures that the update is prevented if the web root and/or vendor directories are not writable but now\Drupal\package_manager\Validator\WritableFileSystemValidatorstops 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.I can see there is a comment
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.
Comment #42
wim leers#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? 😢
Comment #43
kunal.sachdev commentedI tried that strategy and I got 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.
Comment #44
wim leersThe end is in sight! 😄 2 remarks on the MR 😊
Comment #45
kunal.sachdev commentedComment #46
wim leersComment #48
wim leers🐛 Missed something! Overnight,
ComposerValidatorwas made a non-base requirement validator. 🐛Comment #49
wim leersThe issue summary is wildly outdated. Rather than rewriting it, just pointing to the conclusions we reached.
Comment #50
wim leers🚀
Comment #51
phenaproximaFound some things that should be cleaned up before we merge this. But overall it makes sense!
Comment #52
wim leersI think @kunal.sachdev can tackle the remaining feedback 😊
Comment #53
kunal.sachdev commentedComment #54
tedbowLooking good just a few MR comments/suggestions
Comment #55
phenaproximaBack to @tedbow for review.
Comment #56
tedbowNeeds work for test coverage but otherwise looks good
Comment #57
phenaproximaComment #58
phenaproximaComment #59
tedbowThanks everyone!
Comment #60
tedbowThanks everyone!
Comment #62
phenaproximaComment #63
wim leersSo 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 #3342430: Hard failure after module install if composer is not found, which is now unblocked! 🥳