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 #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.
Comment | File | Size | Author |
---|---|---|---|
#15 | drupal.node-requirements.15.patch | 2.75 KB | sun |
#6 | drupal8.node-requirements.6.patch | 2.77 KB | sun |
#5 | drupal8.node-requirements.5.patch | 2.78 KB | sun |
drupal8.node-requirements.0.patch | 543 bytes | sun | |
Comments
Comment #1
lucascaro CreditAttribution: lucascaro commentedI'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:
unless we don't want to run this when update.php is run.
Comment #2
sunNode 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.
Comment #3
lucascaro CreditAttribution: lucascaro commentedAwesome, then this should be RTBC
Comment #4
catchCan 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.
Comment #5
sunSure. Merely wanted to keep the changes minimal in my original patch. Here's the same change with #4 incorporated.
Comment #6
sunbah, trailing white-space.
Comment #7
sunBy the way, that code contradicts itself:
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.
Comment #8
lucascaro CreditAttribution: lucascaro commentedLooks good, same patch but with the request from @catch applied.
Comment #10
chx CreditAttribution: chx commented#6: drupal8.node-requirements.6.patch queued for re-testing.
Comment #11
lucascaro CreditAttribution: lucascaro commentedSorry test bot but this patch has to pass ;) (@sun what happened? did I break it somehow by changing the status?)
Comment #12
sunI 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.
Comment #13
sun#6: drupal8.node-requirements.6.patch queued for re-testing.
Comment #14
webchickThis 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.
Comment #15
sunClean backport to D7.
Comment #16
lucascaro CreditAttribution: lucascaro commentedpatch 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 :)
Comment #17
Chi CreditAttribution: Chi commentedJust wondering, why did we place this function to node.module file not to node.install file as suggested in documentation?
Comment #18
sunI wondered that, too - but that's left for a separate issue :)
Comment #19
webchickYeah, we should open a separate (novice) issue to move this to node.install.
In the meantime, committed and pushed to 7.x. Thanks!