Config Rewrite is overriding Rest Menu Items allowed_menus config

Created on 19 November 2024, 14 days ago

Problem/Motivation

When installing "Lupus Decoupled Menu" module it triggers a config rewrite which replaces the "Rest Menu Items" configs.

The problem is that rest_menu_items 3.0.4 implemented a new config to only expose "allowed menus", which is overridden now by Lupus Decoupled Menu rewriting, therefore no menu is allowed to be exposed by default.

Steps to reproduce

- Update rest_menu_items to 3.0.4 then install "Lupus Decoupled Menu" (this only happens on first install).

Proposed resolution

- Either remove the config_rewrite "https://git.drupalcode.org/project/lupus_decoupled/-/blob/1.x/modules/lu..." if it's not needed as it was implemented to fix this issue https://www.drupal.org/project/lupus_decoupled/issues/3340799 📌 Adopt config_rewrite for install time config Fixed

- Or enable all the menus with code in lupus_decoupled_menu_install

I've tested removing the config/rewrite/rest_menu_items.config.yml and everything looked working fine, so I would suggest that solution if there are no objections from the main project maintainers.

🐛 Bug report
Status

Active

Version

1.0

Component

Code

Created by

🇹🇳Tunisia benellefimostfa

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

Merge Requests

Comments & Activities

  • Issue created by @benellefimostfa
  • 🇹🇳Tunisia benellefimostfa

    Attached a tested working patch, which removes the config_rewrite from the "Lupus Decoupled Menu" module:

  • 🇹🇳Tunisia benellefimostfa

    Removed config_rewrite dependency from .info as well.

  • 🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU

    Disregarding the fact that we need to have a MR instead of a patch, here:

    We looked at this together and I agree that there seems to be no need for config_rewrite to be used (anymore) by this module, and it's just in the way (because it is rewriting new settings that the rest_menu_items has added, to NULL):

  • 🇸🇮Slovenia useernamee Ljubljana

    @roderik here's my take on this:

    I guess we could also slightly change the config_rewrite to replace only the output_values.

    config_rewrite:
      replace: ["output_values"]
    

    But since output_values are the same in the rest_menu_items/config/install and in the lupus_decoupled_menu/config/rewrite I'm not sure why it is even needed. All it does is that it removes

    add_fragment: 1
    base_url: ''
    allowed_menus: {  }
    

    from the rest_menu_items.config which doesn't look desirable.

    That being said. If there's no need to change the values of above config options we don't even need the config_rewrite module in lupus_decoupled at all.

  • Pipeline finished with Success
    13 days ago
    Total: 263s
    #344696
  • 🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU

    @useernamee we discussed this elsewhere but I can't find it. Summarizing just for reference:

    • true, I don't think we need the config_rewrite module at all.
    • Just realizing this now: we can and should remove config_rewrite from the composer.json because lupus_decoupled_menu is the only thing using it.
    • About getting rid of lupus_decoupled_menu completely: that may be well possible -- there are things in the .install file that seem like they are either unnecessary or can be moved into a recipe in the future -- but let's create a separate ticket for that if needed.

    Current status of this ticket:

    I think we all agree this is the way forward. However, our internal integration tests indicate that somehow,

    • the API response for menu items was being cached (2nd request is a cache hit)
    • with this change, it somehow isn't anymore.

    It would be good to look into the reasons for that, so we're sure to know all implications. This ticket is blocked on that - which may take a little while.
    (And also the MR comment.)

Production build 0.71.5 2024