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.
Comment | File | Size | Author |
---|---|---|---|
#15 | upgrade.test.front_.page_.why_.not_.patch | 2.04 KB | Anonymous (not verified) |
#12 | assertion_failures.png | 20.54 KB | xjm |
#11 | upgrade.test.front_.page_.why_.not_.patch | 1.76 KB | Anonymous (not verified) |
#8 | combined.patch | 1.12 KB | catch |
#6 | entity_enabled_test_only.patch | 667 bytes | catch |
Comments
Comment #1
plachThe 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?Comment #2
plachtagging
Comment #3
catchHmm, that looks like the right fix - but why do the upgrade path tests not fail then?
Comment #4
Gábor HojtsyAgreed, looks like the right fix.
Comment #5
webchickEither 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.
Comment #6
catchHere's a test that fails locally, did not try the combined patch but uploading that for the bot as well.
Comment #7
catchOf 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.
Comment #8
catchSecond patch was the wrong one.
Comment #9
webchickHm. 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?
Comment #10
catchI 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.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedupdated patch to add a drupalGet('') and check for some valid content in performUpgrade().
Comment #12
xjmI ran the tests for #11 locally after reverting the fix, and they failed as intended:
That's a pretty small line of code that actually adds a ton of value to the upgrade path tests. :)
Comment #13
xjmchx suggested we should also check for a few other small things, like assertNoRaw for warnings/errors, maintenance theme, etc.
Comment #14
c960657 CreditAttribution: c960657 commentedWithout 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.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedupdated as per #14, and checked that this is enough to address chx's concerns in IRC.
Comment #16
xjmWait, if the process is dying with a fatal error (per #14), then why doesn't that show up as a failure in the test?
Comment #17
tic2000 CreditAttribution: tic2000 commentedBecause the patch also fixes the original issue, hence the update doesn't die with a fatal error.
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedthe 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.
Comment #19
xjmSorry, 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?Comment #20
xjmbeejeebus 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.Comment #21
xjmOkay, never mind. Two things:
update.php
line 412:upgrade.test
:Therefore, I think this is RTBC.
Comment #22
chx CreditAttribution: chx commentedYes, this looks good.
Comment #23
catchOK, 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.
Comment #24.0
(not verified) CreditAttribution: commentedminor corrections