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.

Files: 
CommentFileSizeAuthor
#37 1586166-37.patch1.91 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 40,309 pass(es).
[ View ]
#12 1586166.patch2.28 KBdrumm
PASSED: [[SimpleTest]]: [MySQL] 39,124 pass(es).
[ View ]
#4 1586166.patch2.27 KBdrumm
PASSED: [[SimpleTest]]: [MySQL] 39,100 pass(es).
[ View ]
#1 1586166.patch2.28 KBdrumm
PASSED: [[SimpleTest]]: [MySQL] 39,142 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new2.28 KB
PASSED: [[SimpleTest]]: [MySQL] 39,142 pass(es).
[ View ]

Here is a patch.

Issue tags:+Drupal.org D7, +porting

Here are some tags.

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?

StatusFileSize
new2.27 KB
PASSED: [[SimpleTest]]: [MySQL] 39,100 pass(es).
[ View ]

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

Updated patch attached.

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

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?

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?

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

Issue tags:+sprint 2

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.

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?

StatusFileSize
new2.28 KB
PASSED: [[SimpleTest]]: [MySQL] 39,124 pass(es).
[ View ]

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.

Assigned:drumm» Unassigned

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

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.

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.

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)

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

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?

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.

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

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

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

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

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

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.

Status:Reviewed & tested by the community» Needs work

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

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.

It completely breaks D6->D7 upgrades!

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

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.

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.

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

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.

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

@techatitsbest did you try the workaround in #29?

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

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

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

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

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new1.91 KB
PASSED: [[SimpleTest]]: [MySQL] 40,309 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

#37 works as expected for me.

Works for me too.

Status:Reviewed & tested by the community» Fixed

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