Revealed via #1215104: Use the non-interactive installer in WebTestBase::setUp()

Problem

  • node_requirements() tries to check whether a node access rebuild is required, but hook_requirements() is also invoked during Drupal installation, in which case no database exists yet.
Files: 
CommentFileSizeAuthor
#15 drupal.node-requirements.15.patch2.75 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,303 pass(es).
[ View ]
#6 drupal8.node-requirements.6.patch2.77 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,097 pass(es).
[ View ]
#5 drupal8.node-requirements.5.patch2.78 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,292 pass(es).
[ View ]
drupal8.node-requirements.0.patch543 bytessun
PASSED: [[SimpleTest]]: [MySQL] 37,259 pass(es).
[ View ]

Comments

I'm following this whole simpletest-on-install-profiles issue. I had to do something similar to this to get the installer to work.

Note that this also applies do D7 (I can back port it when this is committed).

I'm not sure about the condition, though, since there could be three phases: install, update and runtime and I think this is supposed to run during update (correct me if I'm wrong) so maybe the best way to filter it would be:

<?php
 
if ($phase == 'install') {
?>

unless we don't want to run this when update.php is run.

Node module's hook_requirements() is only supposed to run on the Status report page, hence the explicit check for $phase 'runtime'.

It's not actually a requirement, but only an informational status row, which is underlined by the fact that it never sets the severity property.

Status:Needs review» Reviewed & tested by the community

Awesome, then this should be RTBC

Status:Reviewed & tested by the community» Needs work

Can we make this a positive check for $runtime, and have a single return $requirements at the end since it'll still only be an array in that case. I don't see a reason to have two returns here. Also the comment around get_t() should be updated since this is never going to run in the installer, or more likely we should no longer use get_t() at all here.

Status:Needs work» Needs review
StatusFileSize
new2.78 KB
PASSED: [[SimpleTest]]: [MySQL] 37,292 pass(es).
[ View ]

Sure. Merely wanted to keep the changes minimal in my original patch. Here's the same change with #4 incorporated.

StatusFileSize
new2.77 KB
PASSED: [[SimpleTest]]: [MySQL] 37,097 pass(es).
[ View ]

bah, trailing white-space.

By the way, that code contradicts itself:

+    // Only show rebuild button if there are either 0, or 2 or more, rows
...
+      'description' => $description . ' ' . l(t('Rebuild permissions'), 'admin/reports/status/rebuild'),

It first goes through expensive count operations and whatnot to figure out whether to show the rebuild button, and then it simply, unconditionally shows it. :P

However, please note that this patch blocks #1215104: Use the non-interactive installer in WebTestBase::setUp(), and therefore I'd really appreciate it if any further/real/actual changes/clean-ups to node_requirements() would be deferred to another issue.

Status:Needs review» Reviewed & tested by the community

Looks good, same patch but with the request from @catch applied.

Status:Reviewed & tested by the community» Needs work
Issue tags:-needs backport to D7, -Testing system

The last submitted patch, drupal8.node-requirements.6.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+needs backport to D7, +Testing system

#6: drupal8.node-requirements.6.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Sorry test bot but this patch has to pass ;) (@sun what happened? did I break it somehow by changing the status?)

I didn't see the actual test failures, but this generally means we have random test failures in HEAD currently - not caused by this patch or issue or any particular action.

#6: drupal8.node-requirements.6.patch queued for re-testing.

Version:8.x-dev» 7.x-dev
Priority:Major» Normal
Status:Reviewed & tested by the community» Patch (to be ported)

This looks pretty straight-forward; just wrapping the existing code in a phase runtime check.

Committed and pushed to 8.x. Marking to 7.x for backport.

I don't think this is major, however, especially in D7, so I'm moving to normal and getting it out of the thresholds.

Status:Patch (to be ported)» Needs review
StatusFileSize
new2.75 KB
PASSED: [[SimpleTest]]: [MySQL] 39,303 pass(es).
[ View ]

Clean backport to D7.

Status:Needs review» Reviewed & tested by the community

patch applies cleanly and it's a direct port of the changes in 8.x. Marking as RTBC since it's only wrapping the code in the same if statement :)

Just wondering, why did we place this function to node.module file not to node.install file as suggested in documentation?

I wondered that, too - but that's left for a separate issue :)

Status:Reviewed & tested by the community» Fixed

Yeah, we should open a separate (novice) issue to move this to node.install.

In the meantime, committed and pushed to 7.x. Thanks!

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