Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
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:
Comments
Comment #4
phenaproximaWrote a simple test. Take a look?
Comment #5
tedbowComment #6
phenaproximaComment #7
tedbowLooks good to me
Comment #8
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
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.
Comment #9
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #10
phenaproximaOK, I converted it to a kernel test and hard-coded the list of menus.
Comment #11
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks. 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.Comment #12
phenaproximaHow 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!
Comment #13
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #14
guilhermevp CreditAttribution: guilhermevp at CI&T commentedI 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.
Comment #16
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks! Pushed to 9.3.x.
Comment #17
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOpened #3222465: Use test config rather than system config in MenuUiNodeTypeTest as a followup.