Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Since Drupal 8.7, Drupal core has been providing a "expand all menu items" checkbox on its menu block: #2594425: Add the option in system menu block to "Expand all items in this tree".
This is essentially a duplicate of the "Expand all menu links" option that this module provides.
The problem is that since this module's menu block extends from core's, both checkboxes appear, which is very confusing. Not only that, but it's impossible to actually enable the checkbox from core, because this module's block doesn't execute core's submit handler, so the config is never saved for that checkbox.
If a goal of this module is to have core eventually adopt its functionality, this is good. This means core as already started doing this. Now the challenge is we need to remove the config option from this module and use core's. This is not so easy because the configs are named differently. Core uses config key "expand_all_items" and this module just uses "expand". So there would need to be a migration path to update config for all usages of this plugin to use the key provided by core. Yikes. Not really practical since block configs can be anywhere these days.
Instead, we should at the very least hide this checkbox from core since it doesn't do anything.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 3133567-use-core-expand-all-items-checkbox-16.patch | 7.81 KB | godotislate |
| |||
| #16 | interdiff_15-16.txt | 1.78 KB | godotislate |
| #15 | interdiff_13-15.txt | 2.55 KB | godotislate |
| #15 | 3133567-use-core-expand-all-items-checkbox-15.patch | 7.58 KB | godotislate |
| |||
| #13 | use-core-expand-all-items-checkbox-3133567-13.patch | 4.94 KB | cainaru |
| |||
Comments
Comment #2
bkosborneLet's try this...
Comment #3
bkosborneComment #5
dwwThanks for opening this! It was reported (out of scope) at #2809699-100: Add configuration options for dynamic block titles. Nice to have a real place for addressing it. :)
Big +1 to removing the duplicate setting.
However, I think I'm -1 to the implementation here. Seems better (even if more complicated) to use core's setting for this, instead of hiding it. Yes, that means we need config migration, but that seems more future-proof than trying to hide core's and keep implementing our own. But I'm on the fence. Pragmatism says a 1.5kb patch (with test coverage!) that fixes a UI bug is a really good thing. Kinda hard to argue with that. ;) Thankfully, I'm not the maintainer, so it's not up to me to decide.
Meanwhile, the fact that we don't call core's submit handler seems problematic. I wonder if there are other settings that are being silently ignored. If so, that'd be another reason to fix this by removing our custom version and actually using core's SystemMenuBlock settings and functionality...
Cheers,
-Derek
Comment #6
AnybodyYep, I agree with #5 - if it's the exactly SAME setting (same expected result), we should definitely use the core setting and merge them. If one of them is enabled, core should be enabled and the custom setting should be removed.
Comment #7
bkosborneI agree it's better to use core's setting. But the update hook will be VERY complicated. If people could only use block plugins via the block UI (placing blocks into regions of a theme), it would be very easy. But people can use block plugins in Layout Builder, where the configuration for the blocks is serialized in the database in some cases, and in other cases is attached to entity view display config entities. There are probably other areas that people are using block configs as well via some other modules that let you embed blocks as paragraphs or in CKEditor or something.
One way to get around this problem is to NOT perform a config update since it's so impractical. Instead:
1) Remove this module's config for "expand" and fix submit handler so it saves the value of core's "expand_all_items" (currently being ignored)
2) When checking for the value of expand, first check if this module's "expand" config is set and use that. Otherwise use the value of core's "expand_all_items"
The downside here of course is that we're basically carrying around technical debt.
Comment #8
joelpittet@bkosborne why is the config update so impractical? I'm leaning towards fixing
expand_all_itemsand updating config that used it to use core's.I'm assuming you'd find all MenuBlock's check the status of
expandand set the value ofexpand_all_itemsto that.Comment #9
joelpittetComment #10
dww@joelpittet re: #8:
Because of exactly what @bkosborne said in #7:
I like the compromise in #7 about having menu_block remain aware of both settings. Perhaps when saving, it can remove the legacy menu_block setting from config? That'd help folks migrate to the new setting over time. Perhaps we could do both, and have a post_update hook for the standard block config, but leave the fallback stuff for all the other block config edge cases? Yes, it means we've got some technical debt to clean up eventually, but it seems better than the technical debt of a really big complicated update that may or may not handle all the edge cases, anyway.
Comment #11
joelpittetYou both have more experience trying to solve the problem than I, I was hoping to do something similar to the update hook in interdiff for #2950943-37: Add links to menu parent block title but have never had to deal with those other config storage locations mentioned. Sorry I must have skipped over that explanation earlier @bkosborne
Comment #12
bkosborneI'll also try to explain it a bit clearer too. There's really three concepts of blocks in core which is confusing:
That's just what core provides. There are other modules, like layout builder, that allow placing block plugins into a layout, and the config for the block plugins can be stored in two different areas for that.
Any contrib module that provides block plugins has this problem of not really being able to provide update paths for configuration of their plugins, because there are an unknown number of areas where the config is stored.
So the update hook example you provided in #11 works great but only for block config entities - AKA blocks placed into a themes regions.
Comment #13
cainaruHere is a first pass at deprecating the "Expand all menu links" checkbox in favor of core's "Expand all menu items" checkbox. It includes a post_update hook for updating standard block config.
Comment #15
godotislate CreditAttribution: godotislate commentedUpdated patch to pass tests. Not sure whether additional tests are required.
Comment #16
godotislate CreditAttribution: godotislate commentedSmall fix for backwards-compatibility with the
expandproperty for blocks in Layout Builder, etc. Also small fixes for variable typing.Comment #17
Kris77 CreditAttribution: Kris77 commentedseems to work for me with last dev version.
Thanks @godotislate
Comment #18
JurgenR CreditAttribution: JurgenR commentedPatch #16 is working, thnx.
Comment #19
jeremyr CreditAttribution: jeremyr commented+1 for patch #16
Comment #20
joelpittetI'm considering committing a solution but I'd like to know how the patch in #16 addresses #10 and #12 points about layout builder and core config updates?
Comment #21
godotislate CreditAttribution: godotislate commentedThere's a post-update hook that updates all global menu block configurations, so that will happen when database updates are run.
There's nothing to update menu blocks in layout builder, and this is mainly due to complexity. It's probably feasible to go through all entity display view defaults layout and find menu blocks within to update them, though to do so is somewhat complex. However, for per-entity overrides, having to go through all content entities to find menu blocks placed in the layout builder field storage and update them might not be ideal, depending on the number of relevant entities.
Backwards-compatibility is provided for the saved settings, but the settings themselves won't get updated unless the blocks are re-saved.
Comment #22
Chris Matthews CreditAttribution: Chris Matthews as a volunteer and at City of Oaks Design commentedChanging status back to RTBC per @godotislate's comments in #21.
Comment #24
joelpittetThanks everybody for your help on this, I've committed it to the dev release for the next release.
On commit, I changed the
array_key_exists()toisset()checks because they are faster and besides NULL values are identical.I have it a manual test to see what hiccups may arise as well.