Problem/Motivation

Follow up to #1882552: Deprecate menu_list_system_menus() and menu_ui_get_menus()

Since menu_ui_form_node_type_form_alter() is no longer calling menu_ui_get_menus() and is instead loading the menus itself and sorting the labels then should add test coverage to make sure the sort actually is removed. That way the new asort() could not be removed.

Proposed resolution

Add test coverage

Remaining tasks

do it

User interface changes

None

API changes

NOne

Data model changes

none

Release notes snippet

none

Issue fork drupal-3221493

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

phenaproxima made their first commit to this issue’s fork.

phenaproxima’s picture

Status: Active » Needs review

Wrote a simple test. Take a look?

tedbow’s picture

Status: Needs review » Needs work
phenaproxima’s picture

Status: Needs work » Needs review
tedbow’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

Instead creating $sorted_menu_names form the menus in the options why not just compare against the hard-coded sorted list?

I agree with this. Also, this would help prevent, for example, the test only testing a list of 1, or a list that happens to be sorted in the database, and therefore appearing sorted even if the sorting code regresses in the future.

it seemed sort of odd to have to couple this test to the config that ships with the System module

I don't think that would be that bad. A patch that changes that config can change the test. Alternatively, in the test setup, we can delete existing menus and create explicit ones defined by the test. That way, we could even be sure to create them in a predefined order that doesn't already happen to be sorted.

effulgentsia’s picture

I also wonder if this would be better as a kernel test that calls the relevant controller and asserts on what's in the returned form/render array (or even renders it if that's necessary or helpful) rather than having to be a browser test. Browser tests are slower and more cumbersome to debug, so should only be used when there's some value that we get that justifies that.

phenaproxima’s picture

Status: Needs work » Needs review

OK, I converted it to a kernel test and hard-coded the list of menus.

effulgentsia’s picture

Thanks. This looks great. I just recommend adding a comment somewhere, whether in a PHPDoc, or an inline comment above $expected_options = [ that what we are testing for is that the menus are sorted alphabetically by their label, not their machine name.

phenaproxima’s picture

How about this -- I included a couple of words in the doc comment which should help, and also just removed the machine names from the expected list entirely. Hopefully that clarifies it!

effulgentsia’s picture

removed the machine names

I liked it better with the machine names in there. The reason is that the purpose of this test is to prevent an accidental removal of the asort() in menu_ui_form_node_type_form_alter(). But if it were removed, then the menus would be sorted alphabetically by their machine name (since that's how config storage sorts). So we need our test case to have a different order when you sort by label than when you sort by machine name. Which, conveniently, system.module happens to have that. We just need a comment that clarifies that.

guilhermevp’s picture

Status: Needs review » Reviewed & tested by the community

I believe the test is pretty clear and complies with the whole discussion thread. Commenting about how it should be ordered by label and not machine is also a nice touch.

  • effulgentsia committed bd04c70 on 9.3.x
    Issue #3221493 by phenaproxima, tedbow, guilhermevp: Add test coverage...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Pushed to 9.3.x.

effulgentsia’s picture

Status: Fixed » Closed (fixed)

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