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.
For Drupal.org, #1559652: Get update.php running smoothly on 7.devdrupal.org, l()
triggers the theme system, which ends up trying to use the filter_format
table, which hasn't been updated to that new name yet. system_update_7013()
and user_update_7002()
are the only places l()
is used in core update functions.
Comment | File | Size | Author |
---|---|---|---|
#37 | 1586166-37.patch | 1.91 KB | David_Rothstein |
#12 | 1586166.patch | 2.28 KB | drumm |
#4 | 1586166.patch | 2.27 KB | drumm |
#1 | 1586166.patch | 2.28 KB | drumm |
Comments
Comment #1
drummHere is a patch.
Comment #2
drummHere are some tags.
Comment #3
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedIs t() available at this stage?
Also, shouldn't
<em>' . check_plain($timezone) . '</em>
be replaced with a %placeholder if the string should now be translated?Comment #4
drummt()
is available and used in other update functions. (It has to be used viaget_t()
on install.)Updated patch attached.
Comment #5
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedLooks good. I'll be brave and set this to RTBC (testbot can set it back if it wants).
Comment #6
chx CreditAttribution: chx commentedThanks much for finding this issue. Why didn't existing tests caught this?
Comment #7
drummDrupal.org ran into it via ctools:
Regardless of ctools, no other update functions call
l()
, so I'm guessing it isn't update-safe; I didn't look around for the official documentation.What's the general strategy for testing that an update functions don't call unsafe functions?
Comment #8
David_Rothstein CreditAttribution: David_Rothstein commentedThis one is missing the call to url().
Comment #9
Senpai CreditAttribution: Senpai commentedComment #10
BTMash CreditAttribution: BTMash commentedLooking at l(), it seems that the theme function gets triggered IF the theme_link variable is set to true. So maybe we just need to enable that variable in one of the dumps to see if we can replicate the issue. I'll see if I can figure out a test for this.
Comment #11
David_Rothstein CreditAttribution: David_Rothstein commentedThe theme_link variable is already TRUE by default.
Seems like writing a test for this would be tricky, because in this case it only happened because Ctools happens to do something that calls filter_formats() while building the theme registry... kind of a random edge case.
Actually, why was Ctools enabled during the D6->D7 upgrade in the first place? "Officially speaking", shouldn't it have been disabled, and only upgraded to D7 later?
Comment #12
drummUpdated patch attached; adding the missing call to
url()
. Drupal.org's update got further with the patch from #4 applied.In practice, I see
t()
being used in quite a few update functions. For example,user.install
, has 4 other uses, including 2 lines later inuser_update_7002()
. Either the documentation needs to be updated or a lot more needs to be patched, in a separate issue.I'll write something about Drupal.org keeping ctools enabled or not in #1559652: Get update.php running smoothly on 7.devdrupal.org.
Comment #13
drummUnassigning myself since I'm not working on tests at this time.
Comment #14
drummIt occurred to me that tests for usage of
t()
andl()
are something coder module would be best at covering, rather than unit tests. It would be something like a rule that checks for one or the other within a hook_update_N() implementation. As I mentioned earlier, we still need to firm up the rules since the code disagrees with documentation fort()
. I'm not aware of an official policy on usingl()
or not in updates; it is only used in the two cases removed by this patch.Comment #15
hass CreditAttribution: hass commentedWould have used, @regional_settings as placeholder, but it doesn't matter for functionality.
Code wise all correct in #12.
Comment #16
chx CreditAttribution: chx commentedEven ordinary bugfixes do not go in without test coverage not to mention update bugs. In this case, the problem to test and guard against is much wider than just
l
: one we must not initialize the theme system during the update! In the test, try to set the registry to an ArrayAccess object which inserts a fail if constructed or any method of it is called. Now I wish we had an expected fail :( Without that, to test this ArrayAccess works, we need to add a test update to a test module which sets a variable and calls theme and the ArrayAccess needs to check that variable, make a pass, and then delete it immediately. So this way we know the ArrayAccess fires but if an update in the future manages to ping theme() this ArrayAccess in the tests will catch it. (You can insert an assert from outside of the test system)Comment #17
chx CreditAttribution: chx commentedFor added fun and games a followup could investigate similiar guarding in module_list (needs a d_s conversion) and m_i.
Comment #18
bfroehle CreditAttribution: bfroehle commentedchx: I've read your envisioned test in #16 a dozen times and am starting to understand what you are talking about. However it seems terribly convoluted, and, as you point out, requires a test just to make sure that the test works. I can't help but think that there must be a simpler way to accomplish this same task. Can we just check the value of a global variable after the update hooks run?
Comment #19
David_Rothstein CreditAttribution: David_Rothstein commentedWill that test ever pass though? I think the theme system is always initialized as as soon as you visit update.php (otherwise we'd never be able to display it as an HTML page in the first place). And the stack trace in #7 seems to show that the problem occurs even when the maintenance theme is being used.
Agreed this is inconsistent overall (and should not be solved here) but if we add new t() strings here, doesn't that mean they'll start showing up on http://localize.drupal.org and tons of people will start translating them into a bunch of languages in preparation for the next release, all for no purpose whatsoever?
I think for that reason it would be better not to add new t() calls. As an alternative it should be possible to use format_string() instead.
Comment #20
catchurl() invokes hooks, so shouldn't be used in updates either.
Comment #21
drummUntagging for D.o -> D7, this was worked around.
Comment #22
lyricnz CreditAttribution: lyricnz commentedI'm amazed this bug is not fixed yet! It completely breaks D6->D7 upgrades!
To workaround, I also had to disable the l() in node_requirements()
Comment #23
jackbravo CreditAttribution: jackbravo commentedThis comment has already been reviewed, should it be marked as RTBC again? The extra tests proposed by @chx seem part of a different issue and this does affect upgrades to D7.
Comment #24
tim.plunkettIt is still missing tests. Tests are not a follow up.
Comment #25
hass CreditAttribution: hass commentedHave we ever added a test for a string change? I don't think so. I have no idea how we can test on the final html code if l() or url() has been used. Sounds impossible to me.
Comment #26
tim.plunkettThat is not a "string change". it is part of the upgrade path and is testable.
Comment #27
lsolesen CreditAttribution: lsolesen commentedI can confirm that the patch solves the error with Table 'drupal.filter_format' doesn't exist. I think tests are a good idea. However, waiting to commit something that makes the update path work again (when it is a rather trivial change) until tests have been written when more people RTBC the patch seems odd to me.
Comment #28
ronline CreditAttribution: ronline commentedPatch doesn't work for my broken update D6 to D7.15.
I am getting “doesn't exist” for filter_formats, (line 304 of /var/www/ronline/www/includes/database/mysql/schema.inc) and “already exist” for tracker_node, blocked_ips, block and comment.
DatabaseSchemaObjectDoesNotExistException: Cannot rename filter_formats to filter_format: table filter_formats doesn't exist. in DatabaseSchema_mysql->renameTable() (line 304 of /var/www/ronline/www/includes/database/mysql/schema.inc).
Taking off the t() from schema.inc on line 304 doesn't fix anything.
Comment #29
drummSpecifically, our workaround was adding
$conf['theme_link'] = FALSE;
to settings.php, we don't want link theming anyway.Comment #30
ronline CreditAttribution: ronline commentedThx for the hint drumm the $conf['theme_link'] = FALSE; doesn't work either.
All update failures have traces of buggy tags mentioned in the comment #3.
Even with patch and $conf['theme_link'] = FALSE I am getting :
DatabaseSchemaObjectDoesNotExistException: Cannot rename filter_formats to filter_format: table filter_formats doesn't exist. in DatabaseSchema_mysql->renameTable() (line 304 of /var/www/ronline/www/includes/database/mysql/schema.inc).
I have tried drupal 7.15 stable and drupal 7-dev.
Comment #31
mrP CreditAttribution: mrP commentedWhats the status here? I just tried a 6.26 to 7.14 upgrade on a trivial site and had this failure.
Comment #32
BTMash CreditAttribution: BTMash commented@techatitsbest did you try the workaround in #29?
Comment #33
mrP CreditAttribution: mrP commented@BTMash -- Thanks for the quick response. The comments were a bit mixed on this so I couldn't tell if there was a confirmed workaround.
To confirm/reiterate #29, adding the following configuration to settings.php resolved the upgrade errors:
Comment #34
dgtlmoon CreditAttribution: dgtlmoon commentedadding that line to my settings.php did not help, it only seems to be an issue for some of my sites, not all of them, might have something todo with the theme settings in general
there needs to be a patch applied to core, (#12 worked for me) it's not real helpful for people to have to trawl the issue queues for some settings when running an upgrade from D6->D7 when we're talking about moving to D8!
Marking this is as critical
Comment #35
tedbowhttp://drupal.org/contributor-tasks/write-issue-summary Link for instructions to create issue summary update
Comment #36
David_Rothstein CreditAttribution: David_Rothstein commentedThis isn't a critical bug because it only happens when you don't follow the instructions in UPGRADE.txt. In order to run into this (I tested) you need to ignore the instructions in two ways:
If you even do one of the above it's still OK and you won't experience the bug. But you definitely can't do both - issues like this are the whole reason the upgrade instructions say not to.
Anyway, we can certainly still fix this but for reasons mentioned above I don't think we should use t(). In addition, if the whole point of removing calls to l() is to avoid initializing a subsystem during the upgrade, then adding t() calls in a new place (which will initialize a new subsystem at a time that it wasn't previously initialized) doesn't seem like a great idea for that reason either, especially at this point in the D7 cycle. We're trying to rock the upgrade boat as little as possible :)
Comment #37
David_Rothstein CreditAttribution: David_Rothstein commentedHere's a reroll which switches to using format_string(). Haven't tested it though.
I'm tentatively removing the "needs tests" tag since I don't think we need tests for something that's not really a bug (but rather only a little gesture for sites that aren't following the instructions in UPGRADE.txt). Still would be happy to commit tests with this if anyone can figure out how to write one, though :)
Comment #38
helmo CreditAttribution: helmo commented#37 works as expected for me.
Comment #39
pebosi CreditAttribution: pebosi commentedWorks for me too.
Comment #40
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/9cc5ed7