- Issue created by @achap
- πΊπΈUnited States smustgrave
Even though categorized as a task, maybe could be feature request, seems like a change that needs test coverage
- achap π¦πΊ
I'm happy to write tests, however before I do maybe someone familiar with the menu system can review the approach and see if this is a sensible change?
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
First and foremost, the elephant in the room: None of the IS is true unless you have a menu link that didn't change being saved frequently. So my first question to you would be: Why do you have menu links being saved without changes so often?
MenuForm::submitOverviewForm()
seems to prevent a menu link from being saved in the tree storage if nothing has changed. So that seems like a double layer of protection there (one in the form, one in the storage) that tries to reduce work.So what is causing a menu link without changes to be saved so frequently outside of the menu form? If we can't answer that, then I fear we might be optimizing something that may not need optimizing just because a website has some other code running that is actually causing havoc.
I'd err on the side of "Closed (won't fix)" if we can't get some better insight into this.
As to the MR:
I'm not sure we can simply change the return of MenuTreeStorage::doSave() like that. We didn't declare the service as internal and it's definitely not unthinkable that there are custom extensions out there that implemented their own ::doSave() to do some extra work in other (perhaps external) systems.
Also not sure about the code flow here:
if ($original) { $affected_menus[$original['menu_name']] = $original['menu_name']; // ... if (array_diff_assoc($fields, $original) == [] && array_diff_assoc($original, $fields) == ['mlid' => $link['mlid']]) { $affected_menus[$original['menu_name']] = FALSE; return $affected_menus; } }
Why keep the original assignment?
I'd go into even further detail, but as I said at the start of my comment, I'm not even sure anything needs to be done here.