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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drumm’s picture

Status: Active » Needs review
FileSize
2.28 KB

Here is a patch.

drumm’s picture

Issue tags: +drupal.org D7, +porting

Here are some tags.

Tor Arne Thune’s picture

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

drumm’s picture

FileSize
2.27 KB

t() is available and used in other update functions. (It has to be used via get_t() on install.)

Updated patch attached.

Tor Arne Thune’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. I'll be brave and set this to RTBC (testbot can set it back if it wants).

chx’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Thanks much for finding this issue. Why didn't existing tests caught this?

drumm’s picture

Drupal.org ran into it via ctools:

SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal.filter_format' doesn't exist
#0  DatabaseConnection->query called at [/var/www/7.devdrupal.org/htdocs/includes/database/select.inc:1264] 
#1  SelectQuery->execute called at [/var/www/7.devdrupal.org/htdocs/modules/filter/filter.module:409]
#2  filter_formats called at [/var/www/7.devdrupal.org/htdocs/modules/filter/filter.module:510] 
#3  filter_default_format called at [/var/www/7.devdrupal.org/htdocs/sites/all/modules/ctools/plugins/content_types/custom/custom.inc:19]
#4  require_once called at [/var/www/7.devdrupal.org/htdocs/sites/all/modules/ctools/includes/plugins.inc:482]
#5  ctools_plugin_load_includes called at [/var/www/7.devdrupal.org/htdocs/sites/all/modules/ctools/includes/plugins.inc:278]
#6  ctools_get_plugins called at [/var/www/7.devdrupal.org/htdocs/sites/all/modules/ctools/includes/content.inc:110]
#7  ctools_get_content_types called at [/var/www/7.devdrupal.org/htdocs/sites/all/modules/ctools/includes/content.theme.inc:15]
#8  ctools_content_theme called at [/var/www/7.devdrupal.org/htdocs/sites/all/modules/ctools/includes/utility.inc:28]
#9  ctools_passthrough called at [/var/www/7.devdrupal.org/htdocs/sites/all/modules/ctools/ctools.module:426]
#10 ctools_theme called at [/var/www/7.devdrupal.org/htdocs/includes/theme.inc:534]
#11 _theme_process_registry called at [/var/www/7.devdrupal.org/htdocs/includes/theme.inc:686]#12 _theme_build_registry called at [/var/www/7.devdrupal.org/htdocs/includes/theme.maintenance.inc:91]
#13 _theme_load_offline_registry
#14 call_user_func_array called at [/var/www/7.devdrupal.org/htdocs/includes/theme.inc:276]
#15 theme_get_registry called at [/var/www/7.devdrupal.org/htdocs/includes/common.inc:2374]
#16 l called at [/var/www/7.devdrupal.org/htdocs/modules/user/user.install:542]
#17 user_update_7002 called at [/usr/local/share/drush/commands/core/drupal/update_7.inc:74]

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?

David_Rothstein’s picture

Component: install system » database update system
  1. +  drupal_set_message(t('The default time zone has been set to %timezone. Check the <a href="@config-url">date and time configuration page</a> to configure it correctly.', array('%timezone' => $timezone, '@config-url' => 'admin/config/regional/settings')), 'warning');
    

    This one is missing the call to url().

  2. In theory, t() is not supposed to be used here either (at least not according to http://drupal.org/node/322731).
Senpai’s picture

Issue tags: +sprint 2
BTMash’s picture

Looking 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.

David_Rothstein’s picture

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

drumm’s picture

FileSize
2.28 KB

Updated 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 in user_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.

drumm’s picture

Assigned: drumm » Unassigned

Unassigning myself since I'm not working on tests at this time.

drumm’s picture

Status: Needs work » Needs review

It occurred to me that tests for usage of t() and l() 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 for t(). I'm not aware of an official policy on using l() or not in updates; it is only used in the two cases removed by this patch.

hass’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/modules/system/system.installundefined
@@ -1976,7 +1976,7 @@ function system_update_7013() {
+  drupal_set_message(t('The default time zone has been set to %timezone. Check the <a href="@config-url">date and time configuration page</a> to configure it correctly.', array('%timezone' => $timezone, '@config-url' => url('admin/config/regional/settings'))), 'warning');

Would have used, @regional_settings as placeholder, but it doesn't matter for functionality.

Code wise all correct in #12.

chx’s picture

Status: Reviewed & tested by the community » Needs work

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

chx’s picture

For added fun and games a followup could investigate similiar guarding in module_list (needs a d_s conversion) and m_i.

bfroehle’s picture

chx: 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?

David_Rothstein’s picture

Will 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.

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 in user_update_7002(). Either the documentation needs to be updated or a lot more needs to be patched, in a separate issue.

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.

catch’s picture

Priority: Normal » Major
Issue tags: +D7 upgrade path

url() invokes hooks, so shouldn't be used in updates either.

drumm’s picture

Issue tags: -drupal.org D7, -porting, -sprint 2

Untagging for D.o -> D7, this was worked around.

lyricnz’s picture

I'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()

jackbravo’s picture

Status: Needs work » Reviewed & tested by the community

This 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.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

It is still missing tests. Tests are not a follow up.

hass’s picture

Have 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.

tim.plunkett’s picture

It completely breaks D6->D7 upgrades!

That is not a "string change". it is part of the upgrade path and is testable.

lsolesen’s picture

I 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.

ronline’s picture

Patch 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.

drumm’s picture

Specifically, our workaround was adding $conf['theme_link'] = FALSE; to settings.php, we don't want link theming anyway.

ronline’s picture

Thx 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.

mrP’s picture

Whats the status here? I just tried a 6.26 to 7.14 upgrade on a trivial site and had this failure.

BTMash’s picture

@techatitsbest did you try the workaround in #29?

mrP’s picture

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

$conf['theme_link'] = FALSE;
dgtlmoon’s picture

Priority: Major » Critical
Status: Needs work » Reviewed & tested by the community

adding 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

tedbow’s picture

http://drupal.org/contributor-tasks/write-issue-summary Link for instructions to create issue summary update

David_Rothstein’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work

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

  1. By updating the contrib modules in your codebase to the 7.x code before the D6-to-D7 core upgrade, rather than after.
  2. By leaving those contrib modules enabled during the D6-to-D7 core upgrade, rather than making sure they are disabled.

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

David_Rothstein’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.91 KB

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

helmo’s picture

Status: Needs review » Reviewed & tested by the community

#37 works as expected for me.

pebosi’s picture

Works for me too.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

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