Enumerated update statuses instead of binary #12
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.
I'm not sure if we need to check for $entry in combination with $status, because it's already checked before with this statement: $status = ($entry && $entry->wasSuccessfulByHook()) ? TRUE : FALSE;
| @@ -20,6 +20,12 @@ class Updater implements UpdaterInterface { | |||
|
|
|||
| use StringTranslationTrait; | |||
|
|
|||
| const CONFIG_NOT_FOUND = 0; | |||
| const CONFIG_ALREADY_APPLIED = 1; | |||
| const CONFIG_NOT_EXPECTED = 2; | |||
There was a problem hiding this comment.
This name is a bit misleading. When I read it, I would expect it's the case when: configuration is there, but it should not be. I was thinking maybe EXPECTED_CONFIG_NOT_COMPLIANT or UNEXPECTED_CONFIG.
What do you think?
| const CONFIG_ALREADY_APPLIED = 1; | ||
| const CONFIG_NOT_EXPECTED = 2; | ||
| const CONFIG_APPLIED_SUCCESSFULLY = 3; | ||
| const MODULES_FOUND = 4; |
There was a problem hiding this comment.
I would leave out module related states since we don't have any log messages related to that right now. And maybe we should add them with PR where we are adding expected modules functionality.
| @@ -130,7 +136,7 @@ protected function logInfo($message) { | |||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
| public function executeUpdate($module, $update_definition_name) { | |||
| public function executeUpdate($module, $update_definition_name, $force = FALSE) { | |||
There was a problem hiding this comment.
Can we keep adding of force functionality outside this PR and add that in another feature request?
| } | ||
| if (empty($expected_configuration) || !DiffArray::diffAssocRecursive($expected_configuration, $config_data)) { | ||
| // Delete configuration keys from config. | ||
| if($checkOnly){ |
There was a problem hiding this comment.
I'm not sure that I understand the purpose of $checkOnly. Is that something like dry-run or?
Additionally, it's not a real representation of an update result, because $config->save(); could throw an exception of real update execution.
| $this->logWarning($this->t('Unable to update configuration for @configName.', ['@configName' => $configName])); | ||
|
|
||
| switch ($result) { | ||
| case Updater::CONFIG_APPLIED_SUCCESSFULLY: |
There was a problem hiding this comment.
Just one quick though - I'm not sure can we somehow propagate $result and send it in ConfigurationUpdateEvent. In that case, we could save it Update entity and use it for some nicer representation of update result.
What do you think?
Enumerated update statuses instead of binary:
This PR will make the module return more meaning full messages when it fails to apply the config updates.