Module creates invalid config dependencies

Created on 5 February 2024, 5 months ago
Updated 27 February 2024, 4 months ago

Problem/Motivation

We recently noticed that block_class is creating dependencies on itself even if no class has been configured on a block.

While this is not necessarily an immediate problem, if the module is uninstalled in the future, any block configs with a dependency on block_class (valid or not) will be deleted. Thus blocks that don't actually have a block class, id or attribute configured will be deleted.

Steps to reproduce

  1. Open a block configuration form for any placed block that does not have a block class, id or attribute configured.
  2. Click 'Save block' without making any changes.
  3. Export configuration and note that the block config now has a dependency on block_class (example below), even though no class was set.
  4. Visit '/admin/modules/uninstall', choose Block Class and click 'Uninstall' — the block above will be listed in the configuration to be deleted if you proceed with the uninstall.

Below is a 'Powered by Drupal' block config created with the steps above. I simply opened the block config form and clicked 'Save block' — you can see that dependency on block_class was created, but no class or other block_class settings were recorded.

langcode: en
status: true
dependencies:
  module:
    - block_class
    - system
  theme:
    - olivero
_core:
  default_config_hash: eYL19CLDyinGTWYQfBD1DswWzglEotE_kHnHx3AxTXM
id: olivero_powered
theme: olivero
region: footer_bottom
weight: 0
provider: null
plugin: system_powered_by_block
settings:
  id: system_powered_by_block
  label: 'Powered by Drupal'
  label_display: '0'
  provider: system
visibility: {  }

Proposed resolution

  • ✅ Recalculate dependencies in the blockClassPreSave() to ensure empty values do not create invalid dependency.
  • ✅ Include an update hook to remove exisiting invalid dependencies.

Remaining tasks

  • ✅ Update/patch
  • Review and test
  • Merge
  • Release

User interface changes

None.

API changes

None.

Data model changes

None.

🐛 Bug report
Status

Needs review

Version

2.0

Component

Code

Created by

🇺🇸United States justcaldwell

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @justcaldwell
  • 🇧🇷Brazil RenatoG Campinas

    Definitely a good catch! Thanks for reporting that @justcaldwell

  • 🇺🇸United States justcaldwell

    Glad to help. I don't have time to dig too deep, but it looks to me like line 358 of blockClassPreSave will always execute and save config, even if all block_class config is empty. Maybe that's where the dependency is created?

  • 🇮🇳India pooja saraah Chennai

    One approach to resolve this could be to add a condition before saving the configuration to check if any `block_class` settings are present. If not, we could skip the save operation to prevent unnecessary dependencies.

  • 🇺🇸United States justcaldwell

    Agreed @pooja saraah, but it seems like the current code is already attempting to do that. At the top of BlockClassHelperService::blockClassPreSave(), each possible setting is checked and unset if empty. I'm not sure why that doesn't address the issue.

  • Status changed to Needs review 4 months ago
  • 🇺🇸United States justcaldwell

    Okay, seems the issue is:

    1. blockClassFormValidate() is setting ThirdPartySettings for block_class, even if the values are empty (e.g., no classes or id were set). This creates the potential for a dependency.
    2. Core's Block::preSave() method runs and calculates dependencies, creating the dependency on block_class based on the (possibly empty) settings above.
    3. blockClassPreSave() unsets any empty values (see #6), BUT it's executed by a hook after the preSave method, and the dependencies don't get recalculated.

    We can either 1) refactor blockClassFormValidate() to avoid setting empty values, or 2) just recalculate the dependencies in the pre-save hook. The MR implements #2 as it's the simplest, and includes an update hook to remove any invalid values.

    This wasn't an issue previously. I don't know what changed to make it start occurring now.

  • 🇺🇸United States justcaldwell

    Updating IS.

Production build 0.69.0 2024