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
DisplayPathTest::testMenuOptions()
and DisplayPathTest::testDefaultMenuTabRegression()
enable the menu_ui
module, after it is already been enabled by the $modules
on the test class. Looking at the original support, this is legacy code that no longer serves a purpose.
Original report
From the documentation of WebTestBase::resetAll:
This method is called by \Drupal\simpletest\WebTestBase::setUp() after enabling the requested modules. It must be called again when additional modules are enabled later.
Proposed resolution
- Remove extra instances of menu_ui
- Add resetAll after additional modules are installed per API
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#16 | 2342807-16.patch | 1.12 KB | Lendude |
#16 | interdiff-2342807-13-16.txt | 759 bytes | Lendude |
#13 | 2342807-13.patch | 631 bytes | Lendude |
#12 | 2342807-12.patch | 613 bytes | Lendude |
#1 | 2342807-call-reset-all.patch | 564 bytes | znerol |
Comments
Comment #1
znerol CreditAttribution: znerol commentedThis patch adds the
resetAll()
which will result in notices thrown from within the page display.Comment #3
dawehnerJust adding tags for now.
Comment #12
LendudeObviously this got moved around a bit and changed to a PHPUnit based test. But the docs for resetAll() are still the same, that this should be called after adding any modules.
But doing so, no longer leads to test fails.
Comment #13
Lendude...and the reason for that might be that the menu_ui module already gets enabled by the $modules....so we can just take this out here.
Comment #14
LendudeComment #15
Kristen PolThanks for the patch update.
1) Confirmed the test has the menu_ui in:
so the additional install code isn't necessary.
2) Patch applies cleanly to 9.2 and 9.1.
3) Shouldn't this issue be set to 9.2 and patch tested against 9.2?
4) There is another places menu_ui is installed:
so this could be removed from the list.
Moving back to needs work for this.
Comment #16
LendudeThanks @Kristen Pol. Removed menu_ui there too, but added the resetAll() since that still adds new modules.
Comment #17
Kristen PolThanks for the update. Marking RTBC based on:
1) Patch applies cleanly to 9.2.
2) Tests pass.
3) Interdiff looks good and fixes the item noticed in #15.
4) I've updated the issue title and summary to reflect the change.
Comment #18
Kristen PolSince this started as a bug and was triaged by the bugsmash team, tagging this.
Comment #19
jibranRTBC++, nice one team.
Comment #21
catchCommitted e36b2f2 and pushed to 9.2.x. Thanks!