This appear to be a very trivial bug: despite system_update_8000() explicitly trying to enable entity, after the D8 upgrade is completed you get a fatal error because entity_get_info() cannot be found. This is simpy due to the fact that the entity module has not been actually enabled.

To reproduce this just upgrade a clean D7 standard installation to D8.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Status: Active » Needs review
FileSize
478 bytes

The attached patch does the trick. On one hand it seems pretty logical to me that a function named update_module_enable() actually enables a module, OTOH the solution appears too easy. Maybe I'm missing something?

plach’s picture

Issue tags: +Upgrade path, +D8 upgrade path

tagging

catch’s picture

Hmm, that looks like the right fix - but why do the upgrade path tests not fail then?

Gábor Hojtsy’s picture

Agreed, looks like the right fix.

webchick’s picture

Issue tags: +Needs tests

Either in this issue or in a critical follow-up, we need to investigate why the existing D8 upgrade tests don't catch this bug, IMO.

catch’s picture

Here's a test that fails locally, did not try the combined patch but uploading that for the bot as well.

catch’s picture

Of course that doesn't explain why the existing tests aren't catching this - ideally we want a generic check for 'site is not completely hosed after upgrade' somewhere.

catch’s picture

FileSize
1.12 KB

Second patch was the wrong one.

webchick’s picture

Hm. Is a better approach for the test to $this->drupalGet() the home page after running the update and make sure the length of the content is > 0 or something?

catch’s picture

I did a drupalGet() in the same place and got a 200. Haven't manually tested the upgrade path with a minimal install to see how it breaks yet.

Anonymous’s picture

updated patch to add a drupalGet('') and check for some valid content in performUpgrade().

xjm’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
20.54 KB

I ran the tests for #11 locally after reverting the fix, and they failed as intended:

assertion_failures.png

That's a pretty small line of code that actually adds a ton of value to the upgrade path tests. :)

xjm’s picture

Status: Reviewed & tested by the community » Needs review

chx suggested we should also check for a few other small things, like assertNoRaw for warnings/errors, maintenance theme, etc.

c960657’s picture

Without this patch, the upgrade process never finished. If you turn on verbose messages on admin/config/development/testing/settings, you can see that the process dies with a fatal error. I suggest adding this check to verify that we actually get to the end of the upgrade process.

@@ -264,7 +264,7 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
 
     // Go!
     $this->drupalPost(NULL, array(), t('Apply pending updates'));
-    if (!$this->assertResponse(200)) {
+    if (!$this->assertText('Updates were attempted.')) {
       return FALSE;
     }
 
Anonymous’s picture

updated as per #14, and checked that this is enough to address chx's concerns in IRC.

xjm’s picture

Wait, if the process is dying with a fatal error (per #14), then why doesn't that show up as a failure in the test?

tic2000’s picture

Because the patch also fixes the original issue, hence the update doesn't die with a fatal error.

Anonymous’s picture

the child process is dying in update.php code, and we're not capturing any errors there.

we were just looking for a 200 http header, which php helpfully sends even on a WSOD. see no evil, report no evil.

xjm’s picture

Because the patch also fixes the original issue, hence the update doesn't die with a fatal error.

Sorry, this is not what I meant. I ran the tests without the fix (see #12), and it seems to me that the fatal should throw its own error, without needing to test for status text.

Is the problem that update.php doesn't jive with simpletest's normal error reporting, outside of actual test assertions?

xjm’s picture

beejeebus looked into #19 and we talked about it in IRC. This is related to another major issue I remember (not going to bother looking it up right now). update.php catches errors and does not report them until later, so that one error doesn't break everything. So, I think we should still check for reported errors as chx suggested.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Okay, never mind. Two things:

  1. This fatal is happening after errors are no longer caught, so we can't check the messages for it. The fatal will just result in whatever the particular server's response to a fatal, be it a 500, a 200 with debug information, or a whitescreen. In update.php line 412:
    // Turn error reporting back on. From now on, only fatal errors (which are      
    // not passed through the error handler) will cause a message to be printed.    
    ini_set('display_errors', TRUE);
    
  2. The other tests I'm asking for are actually already there on the next lines in upgrade.test:
        // Check for errors during the update process.                              
        foreach ($this->xpath('//li[@class=:class]', array(':class' => 'failure')) as $element) {
          $message = strip_tags($element->asXML());
          $this->upgradeErrors[] = $message;
          if ($register_errors) {
            $this->fail($message);
          }
        }
    

Therefore, I think this is RTBC.

chx’s picture

Yes, this looks good.

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK, committed/pushed to 8.x. I had worked on this a bit, but only one line of the tests whicih isn't enough for my "don't commit your own patches" rule.

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

Anonymous’s picture

Issue summary: View changes

minor corrections