Skip to content
This repository has been archived by the owner on Apr 8, 2022. It is now read-only.

Enumerated update statuses instead of binary #12

Open
wants to merge 6 commits into
base: 8.x-1.x
Choose a base branch
from
Open

Enumerated update statuses instead of binary #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
58f13f7
Added enumerated update statuses
waleedq May 4, 2019
66f3162
fixed typo in message
waleedq May 5, 2019
667eb92
added fix for global actions error on check update
waleedq May 6, 2019
6d398f8
fixing already updated check, and removed CONSTANTS from checklist he…
waleedq May 16, 2019
5d743c6
added missing check
waleedq May 16, 2019
c68e1f8
Merge branch '8.x-1.x' into update-statuses
mtodor Jul 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions modules/update_helper_checklist/update_helper_checklist.module
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use Drupal\Core\Url;
use Drupal\update_helper_checklist\UpdateChecklist;
use Symfony\Component\Yaml\Yaml;
use Drupal\update_helper_checklist\Entity\Update;
use Drupal\update_helper\Updater;

/**
* Implements hook_checklistapi_checklist_info().
Expand Down Expand Up @@ -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'])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

$update['#description'] .= $update['#description_successful'];
}
elseif (!$status && !empty($update['#description_failed'])) {
elseif ($entry && !$status && !empty($update['#description_failed'])) {
$update['#description'] .= $update['#description_failed'];
}
}
Expand Down Expand Up @@ -107,6 +108,7 @@ function _update_helper_checklist_checklistapi_checklist_items() {
function update_helper_checklist_modules_installed(array $modules) {
/** @var \Drupal\Core\Extension\ModuleHandler $module_handler */
$module_handler = \Drupal::service('module_handler');
$updateHelper = \Drupal::service('update_helper.updater');

$modules_checklist = [];
$module_directories = $module_handler->getModuleDirectories();
Expand All @@ -120,6 +122,10 @@ function update_helper_checklist_modules_installed(array $modules) {
foreach ($updates_checklist as $version_items) {
foreach ($version_items as $update_hook_name => $checklist_definition) {
if (is_array($checklist_definition)) {

$status = $updateHelper->checkUpdate($module_name, $update_hook_name);
if ($status != Updater::CONFIG_ALREADY_APPLIED && $status != Updater::CONFIG_APPLIED_SUCCESSFULLY) continue;

if (!isset($modules_checklist[$module_name])) {
$modules_checklist[$module_name] = [];
}
Expand Down
126 changes: 104 additions & 22 deletions src/Updater.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_APPLIED_SUCCESSFULLY = 3;
const MODULES_FOUND = 4;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const MODULES_NOT_FOUND = 5;
/**
* Site configFactory object.
*
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep adding of force functionality outside this PR and add that in another feature request?

$this->warningCount = 0;

$update_definitions = $this->configHandler->loadUpdate($module, $update_definition_name);
Expand All @@ -141,7 +147,7 @@ public function executeUpdate($module, $update_definition_name) {
}

if (!empty($update_definitions)) {
$this->executeConfigurationActions($update_definitions);
$this->executeConfigurationActions($update_definitions, $force);
}

// Dispatch event after update has finished.
Expand All @@ -151,6 +157,44 @@ public function executeUpdate($module, $update_definition_name) {
return $this->warningCount === 0;
}

/**
* Check update status of configuration from update definitions.
*
* @param string $module
* Module name where update definition is saved.
* @param string $update_definition_name
* Update definition name. Usually same name as update hook.
*
* @return bool
* Returns update status.
*/
public function checkUpdate($module, $update_definition_name) {
$this->warningCount = 0;
$moduleHandler = \Drupal::service('module_handler');
$modulesInstalled = [];
$update_definitions = $this->configHandler->loadUpdate($module, $update_definition_name);
if (isset($update_definitions[UpdateDefinitionInterface::GLOBAL_ACTIONS])) {
if (isset($update_definitions[UpdateDefinitionInterface::GLOBAL_ACTIONS][UpdateDefinitionInterface::GLOBAL_ACTION_INSTALL_MODULES])) {
$modules = $update_definitions[UpdateDefinitionInterface::GLOBAL_ACTIONS][UpdateDefinitionInterface::GLOBAL_ACTION_INSTALL_MODULES];
foreach ($modules as $module) {
if (!$moduleHandler->moduleExists($module)) return Updater::MODULES_NOT_FOUND;
$modulesInstalled[] = $module;
}
}
unset($update_definitions[UpdateDefinitionInterface::GLOBAL_ACTIONS]);
}

if (!empty($update_definitions)) {
return $this->executeConfigurationActions($update_definitions, FALSE, TRUE);
}

if (empty($update_definitions) && !empty($modulesInstalled)) {
return Updater::MODULES_FOUND;
}

return Updater::CONFIG_NOT_FOUND;
}

/**
* Get array with defined global actions.
*
Expand All @@ -176,8 +220,15 @@ protected function executeGlobalActions(array $global_actions) {
*
* @param array $update_definitions
* List of configurations with update definitions for them.
* @param bool $force
* Force the update.
* @param bool $checkOnly
* Check the update status and don't apply the update.
*
* @return bool
* Returns update status if checkOnly flag is set.
*/
protected function executeConfigurationActions(array $update_definitions) {
protected function executeConfigurationActions(array $update_definitions, $force = FALSE, $checkOnly = FALSE) {
foreach ($update_definitions as $configName => $configChange) {
$update_actions = $configChange['update_actions'];

Expand All @@ -198,15 +249,33 @@ protected function executeConfigurationActions(array $update_definitions) {
$new_config = NestedArray::mergeDeep($new_config, $update_actions['add']);
}

if ($this->updateConfig($configName, $new_config, $configChange['expected_config'], $delete_keys)) {
$this->logInfo($this->t('Configuration @configName has been successfully updated.', ['@configName' => $configName]));
$result = $this->updateConfig($configName, $new_config, $configChange['expected_config'], $delete_keys, $force, $checkOnly);

if ($checkOnly) {
return $result;
}
else {
$this->logWarning($this->t('Unable to update configuration for @configName.', ['@configName' => $configName]));

switch ($result) {
case Updater::CONFIG_APPLIED_SUCCESSFULLY:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

$this->logInfo($this->t('Configuration @configName has been successfully updated.', ['@configName' => $configName]));
break;

case Updater::CONFIG_ALREADY_APPLIED:
$this->logWarning($this->t('Configuration @configName is already updated.', ['@configName' => $configName]));
break;

case Updater::CONFIG_NOT_EXPECTED:
$this->logWarning($this->t('Expected current configuration for @configName is not matching. Unable to apply new config.', ['@configName' => $configName]));
break;

case Updater::CONFIG_NOT_FOUND:
$this->logWarning($this->t('Unable to find config @configName. Skipping update.', ['@configName' => $configName]));
break;
}
}
}


/**
* Installs modules.
*
Expand Down Expand Up @@ -321,41 +390,54 @@ protected function getFlatKeys(array $nested_array) {
* Only if current config is same like old config we are updating.
* @param array $delete_keys
* List of parent keys to remove. @see NestedArray::unsetValue()
* @param bool $force
* Force the update.
* @param bool $checkOnly
* Check the update status and don't apply the update.
*
* @return bool
* Returns TRUE if update of configuration was successful.
*/
protected function updateConfig($config_name, array $configuration, array $expected_configuration = [], array $delete_keys = []) {
protected function updateConfig($config_name, array $configuration, array $expected_configuration = [], array $delete_keys = [], $force = FALSE, $checkOnly = FALSE) {
$config = $this->configFactory->getEditable($config_name);

$config_data = $config->get();

// Reset expected_config in case of force flag.
if ($force) {
$expected_configuration = [];
}

// Check that configuration exists before executing update.
if (empty($config_data)) {
return FALSE;
return Updater::CONFIG_NOT_FOUND;
}

// Check if configuration is already in new state.
$merged_data = NestedArray::mergeDeep($expected_configuration, $configuration);
if (empty(DiffArray::diffAssocRecursive($merged_data, $config_data))) {
return TRUE;
if (!$force && empty(DiffArray::diffAssocRecursive($configuration, $config_data))) {
return Updater::CONFIG_ALREADY_APPLIED;
}

if (!empty($expected_configuration) && DiffArray::diffAssocRecursive($expected_configuration, $config_data)) {
return FALSE;
}
if (empty($expected_configuration) || !DiffArray::diffAssocRecursive($expected_configuration, $config_data)) {
// Delete configuration keys from config.
if($checkOnly){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return Updater::CONFIG_APPLIED_SUCCESSFULLY;
}

// Delete configuration keys from config.
if (!empty($delete_keys)) {
foreach ($delete_keys as $key_path) {
NestedArray::unsetValue($config_data, $key_path);
if (!empty($delete_keys)) {
foreach ($delete_keys as $key_path) {
NestedArray::unsetValue($config_data, $key_path);
}
}
}

$config->setData(NestedArray::mergeDeep($config_data, $configuration));
$config->save();
$config->setData(NestedArray::mergeDeep($config_data, $configuration));
$config->save();

return TRUE;
return Updater::CONFIG_APPLIED_SUCCESSFULLY;
}else{
return Updater::CONFIG_NOT_EXPECTED;
}
}

}