Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

[SD-112] Resolve the issue of the site form layout crashing. #157

Merged

Conversation

vincent-gao
Copy link
Collaborator

@vincent-gao vincent-gao commented Aug 1, 2024

Jira

https://digital-vic.atlassian.net/browse/SD-112

Changes

This PR fixes the recurring layout breakdown issues caused by inconsistencies between active and inactive configurations, which are distributed across different modules.

  • Introduced use of Drupal\Component\Utility\NestedArray for more robust config handling.
  • Implemented a more thorough config update process that,
    • Reads the existing config
    • Merges it with new config changes
    • Deduplicates and sorts dependencies

The root cause of our layout issues came from configurations being spread across multiple modules (e.g. tide_site and tide_site_theming). When update hooks only modify configs in one module (e.g. tide_site), they can inadvertently overwrite settings from other modules (e.g. tide_site_theming), leading to inconsistencies and layout failures.

For instance, content-reference configurations exist in both tide_site and tide_site_theming. Updating only from tide_site can overwrite tide_site_theming settings, causing layout breakdowns.

To mitigate these issues, we should consolidate all field configurations for a module within its top-level module. This approach, combined with proper update methods, can significantly reduce layout disappearance problems. (tide mono-repo 😹 )

@@ -22,7 +24,7 @@ function tide_site_install() {
* New field taxonomy image logo.
*/
function tide_site_update_10001() {
module_load_include('inc', 'tide_core', 'includes/helpers');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we shouldn't use deprecated functions.

$config->set('dependencies', $config_read['dependencies']);
$config->set('content', $config_read['content']);
$config->set('hidden', $config_read['hidden']);
$config->save();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Arbitrarily overwriting active configurations may lead to layout inconsistencies.

$display_config_entity = \Drupal::configFactory()->getEditable($form_config);
$original_config = $display_config_entity->getRawData();
$rewritten_config = NestedArray::mergeDeep($original_config, $rewrite);
if ($rewritten_config['dependencies']) {
Copy link
Collaborator Author

@vincent-gao vincent-gao Aug 1, 2024

Choose a reason for hiding this comment

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

The reason I need to handle the dependencies data seems to be due to a bug with NestedArray::mergeDeep (I'm not entirely sure), which causes data duplication within dependencies. I might need to check Drupal.org for more information it's not a bug, unfortunately.

Copy link
Contributor

@yeniatencio yeniatencio left a comment

Choose a reason for hiding this comment

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

LGTM.

@yeniatencio yeniatencio merged commit 062f3be into develop Aug 5, 2024
4 of 6 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants