-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run hook_requirements() on pm:enable #4733
Run hook_requirements() on pm:enable #4733
Conversation
…row exception for any errors.
…row exception for any errors.
} | ||
} | ||
|
||
throw new \Exception(sprintf("Unable to install module '%s' due to unmet requirement(s):%s", $module, "\n - " . implode("\n - ", $reasons))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Given our long history of not validating this, lets give the user the option to proceed despite the validation error. See https://github.com/drush-ops/drush/blob/10.x/src/Commands/core/UpdateDBCommands.php#L54-L59 for an example from updatedb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All done, it'll default to false
in order to fail by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets default to continue like updatedb does (i.e. no exception). Otherwise we will have to wait for a major version for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the logic and test to reflect that.
And yes, that makes sense since we want to maintain backwards compatibility and avoid breaking any existing integrations/tests which rely on it (but one could also argue that if the module being installed doesn't meet the platform requirements in the first place, there's something wrong with their integration, and the failure should be the appropriate default behaviour, and should lead to them fixing it on their end, and opt out with --yes
).
Also added a bootstrap level of "root" for completeness sake of the "DRUSH_DRUPAL_CORE" constant.
So I take it there are no plans to change the behaviour (in a future major release) so that it fails by default? |
Yeah, I think that’s doable in the next major. The rough thinking is that next major will coincide with Drush 10 which is about a year away. |
This resolves #4190 and supersedes the #4337 PR to include functional tests as well as read all requirement errors.