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.
Revealed via #1373142: Use the Testing profile. Speed up testbot by 50%
Problem
- shortcut_install() calls drupal_installation_attempted() to determine whether a menu_rebuild() is necessary, but drupal_installation_attempted() relies on the MAINTENANCE_MODE constant, which cannot be set when running tests.
Related issues
- The call to drupal_installation_attempted() was introduced in #1187906: Shortcut module cannot be installed via an install profile if the menu module wasn't installed first
- #1177830: shortcut_install() should be in standard_install() attempts to move that code into standard_install().
Comment | File | Size | Author |
---|---|---|---|
#25 | shortcut-menu-rebuild-1376150-25.patch | 6.57 KB | vaibhavjain |
#21 | shortcut-menu-rebuild-1376150-21.patch | 6.57 KB | vaibhavjain |
#19 | shortcut-menu-rebuild-1376150-19.patch | 6.83 KB | vaibhavjain |
#18 | shortcut-menu-rebuild-1376150-18.patch | 7.1 KB | vaibhavjain |
#16 | shortcut-menu-rebuild-1376150-16.patch | 6.57 KB | vaibhavjain |
Comments
Comment #1
sunComment #2
sunRemoved the @todo.
This cannot be tested (or actually, will be tested via the Testing profile change). Looks ready to fly for me.
Comment #3
amateescu CreditAttribution: amateescu commentedLooks like a no-brainer :)
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedUgh... Unfortunate issue.
1. Can't this be tested by adding
protected $profile = 'testing';
to the existing shortcut test cases? Seems like #1373142: Use the Testing profile. Speed up testbot by 50% will effectively do the same thing, but I think it's worth doing here if it's simple.2. I wonder if instead of checking for installation status, could we instead check to make sure the links we're going to save actually correspond to real router items.... I can't remember if there was some reason we didn't do it that way in #1187906: Shortcut module cannot be installed via an install profile if the menu module wasn't installed first but it seems promising.
Don't have time at the moment, but I will try to look into this later today.
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commentedThis issue turned out to be an interesting one.
Here's a patch that I think fixes this in the correct way. We might want to move this issue to the menu system queue...
I tested this manually with an install profile that just installs node + shortcut, plus with the standard install profile, and in all cases everything works. For automated tests, I basically copied the Shortcut module test changes from #1373142: Use the Testing profile. Speed up testbot by 50% and also added a bit to them.
The only problem is that right now, these tests will pass without the rest of the patch too. This is because I had to do this:
This is to work around an issue introduced in #375397: Make Node module optional. This is actually a real bug: If you enable the Node module via the UI at the same time as you install the Shortcut module, you won't get the node-related links in the shortcut bar. So, we should probably fix it rather than work around it in the tests, but I haven't figured out the best way to do that.
Once we fix that, we can put 'shortcut' back in the list of modules passed to parent::setUp(). At that point, the tests should pass with the rest of the patch but lead to a fatal error without it (due to the original bug reported here), which is what we want.
Comment #6
sunI don't really see how we could fix that. The only way I know of would be to make Shortcut depend on Node module, but that would obviously be wrong.
Aside from dependencies, I don't we have any other facility in core that would allow one module to state: "Please install me later, if you happen to install me within a bulk list of modules."
Comment #7
sunEssentially:
"Dependencies between otherwise optional/independent modules to achieve a certain installation order."
I don't think I've seen a package property like that in the debian properties (requires/supports/recommends/conflicts/etc)
A new .info file property?
Comment #8
David_Rothstein CreditAttribution: David_Rothstein commentedI was thinking this might be a good use case for #820054: Add support for recommends[] and fix install profile dependency special casing, actually... Presumably if shortcut had
recommends[] = node
then the module system would also arrange to enable node first when possible.Short of that, playing around with hook_modules_enabled() and hook_modules_installed(), but it could get complicated...
I guess we don't have to deal with that in this issue, as long as we're willing to accept that the test system can't actually catch the bug at the moment (the way it could before Node module was optional).
Comment #9
sunAs @catch nicely described it in http://drupal.org/node/820054#comment-4729660:
Neither recommends[], supports[], nor enhances[] would be correct. They would all imply that there actually should be a user-facing hint on the Modules page.
I'm not sure I fully understand the exact use-case of pre-depends[] in http://www.debian.org/doc/debian-policy/ch-relationships.html - but I can only guess that it implies a hard dependency like depends[] (or our dependencies[])
--
However, you might actually be right -- hook_modules_enabled() gets the full list of $modules being installed.
We only ever want to automatically create those links if a) Node module is already installed, or b) Shortcut and Node module are installed at the same time.
Thus, move that default shortcut set setup in there?
--
But wait. Isn't that code supposed to be entirely moved away in the first place?
#1177830: shortcut_install() should be in standard_install() intends to move it into the Standard installation profile (where it actually belongs).
Comment #10
catchShould we mark this duplicate of #1177830: shortcut_install() should be in standard_install()? And possibly open a separate issue for the optional dependencies stuff?
Comment #11
David_Rothstein CreditAttribution: David_Rothstein commented#1177830: shortcut_install() should be in standard_install() is D8 only, but this needs backport to D7. And I think that other issue has some unresolved UX concerns.
Also, the root issue fixed by this patch is basically in the menu system, so it's worth resolving in D8 either way.
The bug that occurs when node module is enabled at the same time shortcut is installed is the main thing holding this up, but it only affects this issue so much as the tests aren't as nice as we want them to be... that's why I think we could probably avoid dealing with it here. That's also a D8-only bug, so it could be split off to a new issue and/or potentially dealt with by #1177830: shortcut_install() should be in standard_install().
I think based on the above this might make more sense in the menu system queue, although it's really a bit of both...
Comment #12
oriol_e9g1) I think that patch needs a reroll.
2) In assert messages use format_string() instead of t() function (See #500866: [META] remove t() from assert message and http://drupal.org/node/1312890):
Comment #13
cweagansUpdating tags per http://drupal.org/node/1517250
Comment #14
sunGiven that #1177830: shortcut_install() should be in standard_install() landed for D8, I think we should mark this as duplicate and create a new + dedicated issue to have a focused discussion on the architectural problem space that came up here; i.e., how to allow modules to specify an optional dependency in order to ensure a proper order during installation (e.g., of an install profile).
What do you think?
Comment #15
webchickNo, we still need a fix for this issue for D7. But +1 to solving the general problem space. I'd prefer to solve that without the burden of needing to find a backward-compatible solution, though. So moving this one to 7.x. If it turns out the solution reached there is also BC, then we can always remove this fix.
Comment #16
vaibhavjainadding a patch for Drupal 7
Comment #17
larowlanSome minor whitespace issues in here eg:
whitespace
Comment #18
vaibhavjainRemoving whitespaces.
3 instances corrected.
Comment #19
vaibhavjainUpdated patch, some permission changes rectified from previous patch.
Comment #20
larowlanplease use .git/info/exclude for these instead of .gitignore
Comment #21
vaibhavjainRectified, attaching the new patch.
Comment #22
larowlanComment #23
YesCT CreditAttribution: YesCT commented#21: shortcut-menu-rebuild-1376150-21.patch queued for re-testing.
Comment #24
heddn#21 introduces some whitespace errors.
Here.
Here.
Here.
Comment #25
vaibhavjainAttaching the new Diff, should rectify the spacing issues.