Problem/Motivation
The \Drupal\views\Plugin\views\wizard\WizardPluginBase::buildForm()
limits allowed menus to system defined ones if menu_ui module disabled. But menu entities defined by system module and menu selection element (parent menu) is core service menu.parent_form_selector
Also it's a blocker to deprecate remaining function in menu.inc #1882552: Deprecate menu_list_system_menus() and menu_ui_get_menus()
Proposed resolution
- remove check for menu_ui module and inject menu.parent_form_selector
service into wizards
- fix tests - remove non-required menu_ui module from dependencies and add menu_link_content dependency as needed
- review/commit
Remaining tasks
approve, commit
User interface changes
Creating page views allows to select menu (event when menu_ui module disabled)
API changes
Following classes getting new optional dependency on menu.parent_form_selector
service (BC compatible)
- \Drupal\views\Plugin\views\wizard\WizardPluginBase
- \Drupal\views\Plugin\views\display\Page
- \Drupal\node\Plugin\views\wizard\Node
Data model changes
no
Release notes snippet
views no longer require menu_ui module to choose menus
Comment | File | Size | Author |
---|---|---|---|
#48 | interdiff.txt | 4.95 KB | andypost |
#48 | 3021804-43.patch | 18.91 KB | andypost |
Comments
Comment #2
andypostInitial patch
Comment #3
andypostThis function used a lot in contrib http://grep.xnddx.ru/search?text=menu_ui_get_menus
So probably it makes sense to move it out of menu_ui module
Comment #4
LendudeMakes sense to me to remove this check. Wouldn't a check on 'Administer menus and menu items' make much more sense? Because currently this allows you to create menu links even if you are not allowed to administer menus.
If we are calling this a bug, we need a test for this (feels a little feature-y though, probably still good to have a test).
Comment #5
andypost@Lendude Thanks for review - checking for tests I came to this patch
Patch:
- removes mostly all dependency on menu_ui module (regression is not covered by tests - when menu_ui was not enabled)
- removes menu_ui module from tests
Probably issue needs new title kinda - clean-up views menu module usage
Comment #7
andypostFix test & rename vars
Comment #8
andypostFrom BC POV it may need to add throw user exception and this argument should be optional
Comment #9
andypostI think this is required
Comment #10
andypostFiled CR https://www.drupal.org/node/3027559
now it needs tests for menu in wizard and page
Comment #11
scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commentedThe patch in #10 works well for me.
Before patch:
After patch:
I updated the patch to ensure parent menu can be selected in Drupal\Tests\views_ui\Functional\DisplayPathTest, but I am not sure - are we also expecting to see it at /admin/structure/views/add > Create page > Create menu link, because it is not there at the moment.
Comment #12
scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commentedComment #13
scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commentedGoing to look at this a bit further after chatting with @alexpott and @dawehner since in the wizard you cannot select the parent, only the menu, but in the page you can actually select the parent within a menu.
Comment #14
scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commentedOkay Page and Wizard should now match behaviours. Tests added for both. Would be good to review as I am not very confident about this.
Thanks!
Screenshot of wizard page now with option to select a menu parent:
Comment #16
scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commentedAccidentally had something from another issue I was working on in there. Fixed with interdiff from #11 (as interdiff from 14 wouldn't be helpful).
Comment #18
scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commentedUpdated patch
Comment #20
scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commentedHmmm I am a bit stumped, tests are passing on my local. I am thinking it is maybe the conditional field in the wizard not yet showing when running via drupalci.org.
Adding this in case:
Otherwise hoping someone can suggest what else might be going wrong here.
Comment #22
LendudeIssue summary needs an update as well as the title if we want to go with the new direction here, scope creeped pretty radically, not sure if I'm all in on that. The removal of the menu_ui module check has nothing to do with the availability of the parent menu somewhere in the UI. While I'm all for getting the wizard and the menu dialog in line with each other, I'm not convinced this should be done in one issue with the code cleanup. Happy to be convinced that this is a good idea, but please specify in the IS why this would be the case.
No need for the @see, people can just git blame if they want to know
This needs to use $this->assertSession()->, hence the fail as far as I can tell
Comment #23
andypost@scott_euser maybe better to split this issue on deprecation & new feature, it blocks at least 2 issues to deprecate remains #1882552-21: Deprecate menu_list_system_menus() and menu_ui_get_menus()
Comment #24
YurkinPark CreditAttribution: YurkinPark at Skilld commentedChecked 2 notes from message #20, but found one more problem with permissions in test
Comment #26
andypostre-roll and fix messages
Comment #27
andypostReroll after #3067198: Properly deprecate UrlGeneratorTrait
Comment #29
andypostFix new deprecation usage
Comment #31
volegerReroll against 9.0.x
Comment #32
andypostComment #33
daffie CreditAttribution: daffie commentedComment #34
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedComment #35
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch for 9.1.x, please review.
Comment #37
andypostFix reroll and minimize changes, also using
__METHOD__
to have a full namespace in messageComment #38
andypostFix version string
Comment #39
andypostAdd another "BC shim"
Comment #40
andypostComment #44
scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commentedJust a minor change to get the tests passing again, enabling menu_link_content module in test.
Comment #46
andypostReroll for 9.2
Comment #48
andypostre-roll for 9.3
Comment #49
andypost@lendude re: #22
This is not about parent selection but about usage of
menu.parent_form_selector
service which now a part of core, so no reason to depend on menu_ui anymoreComment #50
longwaveLooks OK to me, this injects the service properly in the two places it is used, converting the Views wizard at the same time. There is added test coverage as well so I think this is good to go.
Comment #52
catchCommitted 7a5addd and pushed to 9.3.x. Thanks!
Comment #53
andypostThank you very much! It unblocking #1882552: Deprecate menu_list_system_menus() and menu_ui_get_menus()