-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
…lper, added check for installed modules
@@ -62,10 +63,10 @@ function _update_helper_checklist_checklistapi_checklist_items() { | |||
$entry = Update::load($update_key); | |||
|
|||
$status = ($entry && $entry->wasSuccessfulByHook()) ? TRUE : FALSE; | |||
if ($status && !empty($update['#description_successful'])) { | |||
if ($entry && $status && !empty($update['#description_successful'])) { |
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.
Does this look like a leftover from PR #12 ?
@@ -20,6 +20,12 @@ class Updater implements UpdaterInterface { | |||
|
|||
use StringTranslationTrait; | |||
|
|||
const CONFIG_NOT_FOUND = 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.
Same as for update_helper_checklist.module
, it's again similar to changes in PR #12.
|
||
use LoggerAwareTrait; | ||
|
||
public function __construct() { |
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.
Probably we don't need an empty constructor.
use Psr\Log\LoggerAwareInterface; | ||
use Psr\Log\LoggerAwareTrait; | ||
|
||
class CommandHelper implements LoggerAwareInterface { |
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.
Maybe we could make a service from this? Then we could nicely inject update_helper.updater
and also we could inject it in ApplyConfigurationUpdateCommands
for Drush services.
@@ -0,0 +1,5 @@ | |||
services: | |||
update_helper.commands: |
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.
This service name does not match the class name. Probably class ApplyConfigurationUpdateCommands
should be renamed into something like UpdateHelperCommands
if we want one class to handle multiple commands. Or we should rename service name and class to match single command per class approach.
I'm not sure what is best practice in case of Drush commands?
* ApplyConfigurationUpdateCommands constructor. | ||
*/ | ||
public function __construct() { | ||
$this->commandHelper = new CommandHelper(); |
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 would be good to inject this as service.
This was addressed as part of #23 |
A drush command to run updates through command line, The drush command have --force flag to skip expected config and apply the config updates forcefully.