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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lucascaro’s picture

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:

  if ($phase == 'install') {

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

sun’s picture

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.

lucascaro’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, then this should be RTBC

catch’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
2.78 KB

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

sun’s picture

bah, trailing white-space.

sun’s picture

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.

lucascaro’s picture

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.

chx’s picture

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

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

lucascaro’s picture

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

sun’s picture

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.

sun’s picture

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

webchick’s picture

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.

sun’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.75 KB

Clean backport to D7.

lucascaro’s picture

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

Chi’s picture

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

sun’s picture

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

webchick’s picture

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.