-
-
Notifications
You must be signed in to change notification settings - Fork 555
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
[update:execute] run trailing post updates if there is no updates #3996
Conversation
do you think this can be merged ? |
dc62373
to
94cb713
Compare
composer.json
Outdated
@@ -41,7 +41,7 @@ | |||
"composer/installers": "~1.0", | |||
"doctrine/annotations": "^1.2", | |||
"doctrine/collections": "^1.3", | |||
"drupal/console-core": "1.8.0", |
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.
Can you revert this changes ? The version should be fixed
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.
done
if (!$this->getUpdates()) { | ||
$postUpdates = $this->postUpdateRegistry->getPendingUpdateInformation(); | ||
|
||
if(!empty($postUpdates)) { |
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.
Space is missing
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.
fixed
if (!$this->getUpdates()) { | ||
$postUpdates = $this->postUpdateRegistry->getPendingUpdateInformation(); |
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.
It looks like you set maintenance mode, but it will be reverted only when where are $postUpdates. In other case the website will stay in maintenance mode.
Actually, there is better mechanism for maintenance mode in the Console core, it is merged, but it is not tagged yet. I asked to create a new tag.
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.
Check the way we will activate maintenance mode
protected function configure()
{
$this
->setName('update:entities')
->setDescription($this->trans('commands.update.entities.description'))
->enableMaintenance()
->setAliases(['upe']);
}
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.
it looks much better with this new helper.
I had updated the code, do you want me to rebase the pr ?
@@ -271,9 +281,8 @@ private function runUpdates( | |||
/** | |||
* @return bool | |||
*/ | |||
private function runPostUpdates() | |||
private function runPostUpdates($postUpdates) |
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.
Based on the comment above, we can keep it as it was
@lalop Thank you for your work, no need to rebase. I will merge as soon as we set a new tag on the Drupal console core |
@LOBsTerr do you have any idea of when does drupal console core will be tagged ? |
I have merged it will be tagged soon. |
We can have trailing postUpdates to run or entities update without having any module update.
This pr try to fix that