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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Issue tags: +Testing system
sun’s picture

Removed the @todo.

This cannot be tested (or actually, will be tested via the Testing profile change). Looks ready to fly for me.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks like a no-brainer :)

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

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

David_Rothstein’s picture

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

-    parent::setUp('toolbar', 'shortcut');
+    parent::setUp('node', 'toolbar');
+
+    // Enable the Shortcut module separately, so we can guarantee that it will
+    // be enabled after the Node module. This allows the node-related links in
+    // shortcut_install() to be created.
+    module_enable(array('shortcut'));

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.

sun’s picture

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.

I 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."

sun’s picture

Essentially:

"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?

install_after[] = node
David_Rothstein’s picture

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

sun’s picture

As @catch nicely described it in http://drupal.org/node/820054#comment-4729660:

use recommends[] to mean that there is not a hard dependency (i.e. allow those modules to be disabled), but if they're available in the file system, enable them at the same time if they're not already enabled.

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

catch’s picture

Should we mark this duplicate of #1177830: shortcut_install() should be in standard_install()? And possibly open a separate issue for the optional dependencies stuff?

David_Rothstein’s picture

Title: Shortcut module installation fails in tests when installed later » Shortcut module installation fails in tests when installed later (due to menu system not saving menu links correctly)
Component: shortcut.module » menu system

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

oriol_e9g’s picture

Status: Needs review » Needs work

1) 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):

$this->assertTrue(count($this->set->links), format_string('Found @count links in the default shortcut set.', array('@count' => count($this->set->links))));
foreach ($this->set->links as $link) {
  $this->assertFieldByXPath('//div[@class="toolbar-shortcuts"]//a[contains(@href, "' . $link['link_path'] . '")]', NULL, format_string('Shortcut for @href found in the toolbar.', array('@href' => $link['link_path'])));
}
cweagans’s picture

Issue tags: +Needs backport to D7
sun’s picture

Given 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?

webchick’s picture

Version: 8.x-dev » 7.x-dev

No, 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.

vaibhavjain’s picture

Status: Needs work » Needs review
FileSize
6.57 KB

adding a patch for Drupal 7

larowlan’s picture

Status: Needs review » Needs work

Some minor whitespace issues in here eg:

+++ b/includes/menu.incundefined
@@ -3091,6 +3091,20 @@ function menu_link_save(&$item, $existing_item = array(), $parent_candidates = a
+  }
+  ¶

whitespace

vaibhavjain’s picture

Status: Needs work » Needs review
FileSize
7.1 KB

Removing whitespaces.
3 instances corrected.

vaibhavjain’s picture

Updated patch, some permission changes rectified from previous patch.

larowlan’s picture

Status: Needs review » Needs work
+++ b/.gitignoreundefined
@@ -4,3 +4,4 @@ sites/*/settings*.php
 sites/*/private
+/nbproject/private/
\ No newline at end of file

please use .git/info/exclude for these instead of .gitignore

vaibhavjain’s picture

Rectified, attaching the new patch.

larowlan’s picture

Status: Needs work » Needs review
YesCT’s picture

heddn’s picture

Status: Needs review » Needs work

#21 introduces some whitespace errors.

+++ b/includes/menu.incundefined
@@ -3091,6 +3091,20 @@ function menu_link_save(&$item, $existing_item = array(), $parent_candidates = a
+  ¶

Here.

+++ b/includes/menu.incundefined
@@ -3138,17 +3152,7 @@ function menu_link_save(&$item, $existing_item = array(), $parent_candidates = a
+  ¶

Here.

+++ b/modules/shortcut/shortcut.testundefined
@@ -110,6 +123,17 @@ class ShortcutLinksTestCase extends ShortcutTestCase {
+  }
+  ¶
+  /**
    * Tests that creating a shortcut works properly.
    */
   function testShortcutLinkAdd() {
@@ -150,6 +174,9 @@ class ShortcutLinksTestCase extends ShortcutTestCase {

@@ -150,6 +174,9 @@ class ShortcutLinksTestCase extends ShortcutTestCase {
    * Tests that the "add to shortcut" link changes to "remove shortcut".
    */
   function testShortcutQuickLink() {
+    theme_enable(array('seven'));
+    variable_set('admin_theme', 'seven');
+    variable_set('node_admin_theme', TRUE);
     $this->drupalGet($this->set->links[0]['link_path']);
     $this->assertRaw(t('Remove from %title shortcuts', array('%title' => $this->set->title)), '"Add to shortcuts" link properly switched to "Remove from shortcuts".');
   }

Here.

vaibhavjain’s picture

Attaching the new Diff, should rectify the spacing issues.