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

CommentFileSizeAuthor
#48 interdiff.txt4.95 KBandypost
#48 3021804-43.patch18.91 KBandypost
#46 3021804-46.patch18.91 KBandypost
#44 interdiff-3021804-39-44.txt401 bytesscott_euser
#44 drupal-remove-check-for-menu-ui-3021804-44.patch20.02 KBscott_euser
#39 3021804-39.patch20.06 KBandypost
#39 interdiff.txt1.22 KBandypost
#38 3021804-38.patch19.69 KBandypost
#38 interdiff.txt1.18 KBandypost
#37 3021804-37.patch19.69 KBandypost
#37 interdiff.txt4.13 KBandypost
#35 3021804-35.patch19.89 KBmrinalini9
#31 3021804-31.patch19.86 KBvoleger
#29 drupal-remove-check-for-menu-ui-3021804-29.patch20.08 KBandypost
#29 interdiff.txt2.34 KBandypost
#27 drupal-remove-check-for-menu-ui-3021804-27.patch17.74 KBandypost
#26 drupal-remove-check-for-menu-ui-3021804-26.patch17.74 KBandypost
#26 interdiff.txt3.48 KBandypost
#24 interdiff-3021804-20-24.txt1.58 KBYurkinPark
#24 drupal-remove-check-for-menu-ui-3021804-24.patch17.74 KBYurkinPark
#20 interdiff-3021804-18-20.txt1.07 KBscott_euser
#20 drupal-remove-check-for-menu-ui-3021804-20.patch17.34 KBscott_euser
#20 drupal-3021804-20-tests-passing.png81.51 KBscott_euser
#18 interdiff-3021804-16-18.txt1.15 KBscott_euser
#18 drupal-remove-check-for-menu-ui-3021804-18.patch17.22 KBscott_euser
#16 interdiff-3021804-11-16.txt6.56 KBscott_euser
#16 drupal-remove-check-for-menu-ui-3021804-16.patch16.52 KBscott_euser
#14 screenshot-drupal8.ddev_.local-2019.01.26-16-14-12.png93.89 KBscott_euser
#14 drupal-remove-check-for-menu-ui-3021804-14.patch26.46 KBscott_euser
#14 interdiff-3021804-11-14.txt16.32 KBscott_euser
#11 3021804-10-11.txt1.36 KBscott_euser
#11 drupal-remove-check-for-menu-ui-3021804-11.patch12.92 KBscott_euser
#11 screenshot-drupal8.ddev_.local-2019.01.26-14-49-53.png58.39 KBscott_euser
#11 screenshot-drupal8.ddev_.local-2019.01.26-14-50-36.png62 KBscott_euser
#10 3021804-10.patch11.74 KBandypost
#10 interdiff.txt1.15 KBandypost
#9 3021804-9.patch11.56 KBandypost
#9 interdiff.txt1.39 KBandypost
#7 3021804-7.patch11.36 KBandypost
#7 interdiff.txt3.79 KBandypost
#5 3021804-5.patch10.02 KBandypost
#5 interdiff.txt9.6 KBandypost
#2 3021804-2.patch1.4 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost’s picture

This 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

Lendude’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Makes 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).

andypost’s picture

Status: Needs work » Needs review
FileSize
9.6 KB
10.02 KB

@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

Status: Needs review » Needs work

The last submitted patch, 5: 3021804-5.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review
FileSize
3.79 KB
11.36 KB

Fix test & rename vars

andypost’s picture

+++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -55,7 +55,7 @@ class Page extends PathPluginBase {
-  protected $parentMenuSelector;
+  protected $parentFormSelector;

From BC POV it may need to add throw user exception and this argument should be optional

andypost’s picture

FileSize
1.39 KB
11.56 KB

I think this is required

andypost’s picture

FileSize
1.15 KB
11.74 KB

Filed CR https://www.drupal.org/node/3027559

now it needs tests for menu in wizard and page

scott_euser’s picture

The patch in #10 works well for me.

Before patch:
menu select parent in view before patch

After patch:
menu select parent in view before 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.

scott_euser’s picture

scott_euser’s picture

Assigned: Unassigned » scott_euser
Status: Needs review » Needs work

Going 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.

scott_euser’s picture

Okay 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:
Wizard view page option to select parent

Status: Needs review » Needs work

The last submitted patch, 14: drupal-remove-check-for-menu-ui-3021804-14.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

scott_euser’s picture

Accidentally had something from another issue I was working on in there. Fixed with interdiff from #11 (as interdiff from 14 wouldn't be helpful).

Status: Needs review » Needs work

The last submitted patch, 16: drupal-remove-check-for-menu-ui-3021804-16.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

scott_euser’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
17.22 KB
1.15 KB

Updated patch

Status: Needs review » Needs work

The last submitted patch, 18: drupal-remove-check-for-menu-ui-3021804-18.patch, failed testing. View results

scott_euser’s picture

Hmmm 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:

+    // Wait for conditional field to show.
+    $page->waitForElementVisible('page[link_properties][parent]');

Otherwise hoping someone can suggest what else might be going wrong here.

Tests passing on my local

Status: Needs review » Needs work

The last submitted patch, 20: drupal-remove-check-for-menu-ui-3021804-20.patch, failed testing. View results

Lendude’s picture

Issue 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.

  1. +++ b/core/modules/views_ui/tests/src/Functional/DisplayPathTest.php
    @@ -249,6 +243,24 @@ public function testDefaultMenuTabRegression() {
    +    // @see https://www.drupal.org/project/drupal/issues/3021804
    

    No need for the @see, people can just git blame if they want to know

  2. +++ b/core/modules/views_ui/tests/src/FunctionalJavascript/ViewsWizardTest.php
    @@ -53,6 +53,15 @@ public function testCreateViewWizard() {
    +    $page->waitForElementVisible('page[link_properties][parent]');
    

    This needs to use $this->assertSession()->, hence the fail as far as I can tell

andypost’s picture

@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()

YurkinPark’s picture

Checked 2 notes from message #20, but found one more problem with permissions in test

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

andypost’s picture

Status: Needs review » Needs work

The last submitted patch, 27: drupal-remove-check-for-menu-ui-3021804-27.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review
FileSize
2.34 KB
20.08 KB

Fix new deprecation usage

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

voleger’s picture

Reroll against 9.0.x

andypost’s picture

Issue tags: +Needs reroll
daffie’s picture

Status: Needs review » Needs work
mrinalini9’s picture

Assigned: Unassigned » mrinalini9
mrinalini9’s picture

Assigned: mrinalini9 » Unassigned
Status: Needs work » Needs review
FileSize
19.89 KB

Rerolled patch for 9.1.x, please review.

Status: Needs review » Needs work

The last submitted patch, 35: 3021804-35.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

Fix reroll and minimize changes, also using __METHOD__ to have a full namespace in message

andypost’s picture

FileSize
1.18 KB
19.69 KB

Fix version string

andypost’s picture

FileSize
1.22 KB
20.06 KB

Add another "BC shim"

andypost’s picture

Issue tags: -Needs reroll

The last submitted patch, 37: 3021804-37.patch, failed testing. View results

The last submitted patch, 38: 3021804-38.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 39: 3021804-39.patch, failed testing. View results

scott_euser’s picture

Status: Needs work » Needs review
FileSize
20.02 KB
401 bytes

Just a minor change to get the tests passing again, enabling menu_link_content module in test.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

andypost’s picture

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

andypost’s picture

Title: Remove check for menu_ui module in \Drupal\views\Plugin\views\wizard\WizardPluginBase » Remove optional dependency on menu_ui module in \Drupal\views\Plugin\views\wizard\WizardPluginBase
Issue summary: View changes
Issue tags: -Needs issue summary update, -Needs title update

@lendude re: #22

+++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -311,36 +327,24 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-          $form['menu']['parent'] = \Drupal::service('menu.parent_form_selector')->parentSelectElement($menu_parent, $menu_link);
...
-            '#markup' => $this->t('Menu selection requires the activation of Menu UI module.'),

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 anymore

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Looks 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.

  • catch committed 7a5addd on 9.3.x
    Issue #3021804 by andypost, scott_euser, YurkinPark, mrinalini9, voleger...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 7a5addd and pushed to 9.3.x. Thanks!

andypost’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.