The conversion from 6.x-style tokens (eg !username) to 7.x-style tokens (eg [user:name]) is attempted incorrectly.

Patch for user_update_7011() attached, that:
- corrects the 6.x tokens
- corrects the select query condition, 'value' was used instead of 'name'
- use variable_get()/variable_set() so the serialization is done properly
- adds !date to [date:medium] token update (!date is supported under 6.x)

Files: 
CommentFileSizeAuthor
#33 user-update-7011-1014262-33.patch8.56 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 32,062 pass(es).
[ View ]
#29 user_reroll.patch8.45 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_reroll.patch.
[ View ]
#26 interdiff.patch484 bytesEric_A
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_6.patch.
[ View ]
#26 user-update-7011-1014262-26.patch8.45 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 32,022 pass(es).
[ View ]
#23 user-update-7011-1014262-23.patch8.45 KBchx
PASSED: [[SimpleTest]]: [MySQL] 32,023 pass(es).
[ View ]
#23 interdiff.patch888 byteschx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_5.patch.
[ View ]
#21 user-update-7011-tests-only-1014262-21.patch4.53 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 32,077 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#21 user-update-7011-1014262-21.patch8.59 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 32,074 pass(es).
[ View ]
#19 user-update-7011-tests-only-1014262-19.patch4.53 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 32,074 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#19 user-update-7011-1014262-19.patch6.92 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 32,041 pass(es).
[ View ]
#13 user-update-7011-tests-only-1014262-13.patch4.38 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 31,796 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#13 user-update-7011-1014262-13.patch6.77 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 31,803 pass(es).
[ View ]
#12 user-update-7011-tests-only-1014262-12.patch4.39 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 31,805 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#12 user-update-7011-1014262-12.patch6.77 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 31,799 pass(es).
[ View ]
#11 user-update-7011-tests-only-1014262-11.patch3.75 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 31,742 pass(es).
[ View ]
#11 user-update-7011-1014262-11.patch6.13 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 31,743 pass(es).
[ View ]
#9 user-update-7011-tests-only-1014262-9.patch4.9 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 31,771 pass(es), 4 fail(s), and 0 exception(es).
[ View ]
#9 user-update-7011-1014262-9.patch7.28 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 31,772 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#5 user-update-7011-tests-only-1014262-5.patch2.44 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 31,732 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#5 user-update-7011-1014262-5.patch4.82 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 31,739 pass(es).
[ View ]
user-update-7011.patch2.55 KBasimmonds
PASSED: [[SimpleTest]]: [MySQL] 31,681 pass(es).
[ View ]

Comments

user-update-7011.patch queued for re-testing.

Comment copied from #1137132: Upgrade from D6 to D7, account activation emails not upgraded.

The problem with just replacing the tokens as in the above patch is that simply replacing the tokens is inadequate.

The D6 automated messages sent the user's password via plaintext. D7 does away with this. So not only is there no token for !password in D7, but the email would make little sense. For example, the D6 automated email for a new account says:

!username,

A site administrator at !site has created an account for you.

You may now log in to !login_uri using the following username and password:

username: !username
password: !password

...

If we just replace the tokens, a user will get an email that purports to tell them their password but has nothing there. This is horribly confusing for users and would require manual intervention. Hence the reason I suggested that during upgrade we check to see if the user still has the default (unmodified) D6 emails. If so, we can just wipe them and replace them with the new D7 appropriate ones. If not, do a token replacement but warn the user that their messages will need updating.

Or, in the alternative, it may make more sense to just overwrite the D6 automated messages with the D7 ones entirely. This would be much easier to code (since we no longer have to try to figure out if they match the D6 defaults) but could be potentially destructive if people have crafted such messages. Though I'd say worth it because it'd be far easier to tweak messages with the correct tokens than it was to piece-by-piece manually swap all of the tokens and rewrite the email to make sense like I had to do.

In my case it was quite embarrassing to get an email from a user wondering what the heck their account activation email meant. Turned out about a dozen had gone out like that before someone let me know. At a bare minimum, this issue ought to be mentioned in UPGRADE.txt.

Issue tags:+Needs tests

Where would a test for this be added? I guess a new file modules/simpletest/tests/upgrade/upgrade.user.test?

Priority:Normal» Major
StatusFileSize
new4.82 KB
PASSED: [[SimpleTest]]: [MySQL] 31,739 pass(es).
[ View ]
new2.44 KB
FAILED: [[SimpleTest]]: [MySQL] 31,732 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Here's a start in adding a test to asimmonds' work.

The confusion here came from #554088: Move user_mail_tokens() and emails to token system which first changed !site to [site:name], and then #660302: Migration path for changed user email tokens; don't complicate translation of default messages which converted [site:name] to !site-name-token and then back to [site:name]. (And similarly for the other tokens.)

A bit misleading perhaps: the one test I added is about a saved (modified) text, even if I used a default text.

We need to add some more saved texts/ tokens and also test cases where no text was saved, notably the case where we show a message about the old password token.

I don't think I'm going to work on this in the next 24 hours. Feel free to continue ;-)

I've tagged two issues as duplicate. We want this fixed way before everybody starts upgrading...

Title:User email template tokens not upgradedUser email template tokens not upgraded properly

We need to add some more saved texts/ tokens and also test cases where no text was saved, notably the case where we show a message about the old password token.
No need to test cases where no text was saved. The default texts with password tokens have adequate default replacements.

Title:User email template tokens not upgraded properlyUser email template tokens not upgraded
StatusFileSize
new7.28 KB
FAILED: [[SimpleTest]]: [MySQL] 31,772 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
new4.9 KB
FAILED: [[SimpleTest]]: [MySQL] 31,771 pass(es), 4 fail(s), and 0 exception(es).
[ View ]

Title:User email template tokens not upgraded properlyUser email template tokens not upgraded
Status:Needs review» Needs work

The last submitted patch, user-update-7011-1014262-9.patch, failed testing.

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new6.13 KB
PASSED: [[SimpleTest]]: [MySQL] 31,743 pass(es).
[ View ]
new3.75 KB
PASSED: [[SimpleTest]]: [MySQL] 31,742 pass(es).
[ View ]

The performUpgrade() method does not leave us with the final upgrade status messages. I've removed the lines that were asserting these upgrade messages. We now only assert the actual upgrade. I also added a message to these assertions.

StatusFileSize
new6.77 KB
PASSED: [[SimpleTest]]: [MySQL] 31,799 pass(es).
[ View ]
new4.39 KB
FAILED: [[SimpleTest]]: [MySQL] 31,805 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

I had managed to remove the simpletest.info file from the last patches...

Title:User email template tokens not upgradeduser_update_7011() completely broken (User email template tokens not upgraded)
StatusFileSize
new6.77 KB
PASSED: [[SimpleTest]]: [MySQL] 31,803 pass(es).
[ View ]
new4.38 KB
FAILED: [[SimpleTest]]: [MySQL] 31,796 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Removed t() from assert message.

Priority:Major» Critical

This really needs some more eyes. A 7.1 release with the upgrade path still broken this way... Sites suddenly sending registration mails with non-existing tokens is just ... unthinkable...

Subscribe...
"Sites suddenly sending registration mails with non-existing tokens is just ... unthinkable..."

Subscribe - I assumed I did something wrong in my upgrade and tried to fix the password change form - all I managed to do was make it worse. Is there a level higher than critical? :)

Status:Needs review» Needs work

Instead of user and user-2 in the tests, could we have user-password-token and user-no-password-token or similar?

What happens to sites that are already upgraded from D6? Shouldn't this be a new update function so that it runs again?

Otherwise looks fine.

yikes. subscribing.

Status:Needs work» Needs review
StatusFileSize
new6.92 KB
PASSED: [[SimpleTest]]: [MySQL] 32,041 pass(es).
[ View ]
new4.53 KB
FAILED: [[SimpleTest]]: [MySQL] 32,074 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

These are the patches from #13 with renamed classes and files (See #17).
A version with an rerun I haven't done yet, but it seems like a very nice to have and very low risk. Should it check the schema version or just run the same code once more?

It probably makes sense to just copy the code to a new update handler, then zero out this one, leaving a comment to explain what happened. That way it won't actually run twice on 6-7 upgrades.

Everything else looks fine to me.

StatusFileSize
new8.59 KB
PASSED: [[SimpleTest]]: [MySQL] 32,074 pass(es).
[ View ]
new4.53 KB
FAILED: [[SimpleTest]]: [MySQL] 32,077 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

It feels a bit wrong to me to just zero out user_update_7011(). It was completely broken by accident and now on purpose. In theory there could even be update dependencies declared.
Well, here's a version of this approach to look at, anyways.

Other changes:
These patches now set the file mode to 100644 (instead of 100755).
"Updates email templates to use new tokens." => "Update email templates to use new tokens."

+ * tokens, user_update_7016() now targets email templates of Drupal 6 sites and
should have been
+ * tokens, user_update_7017() now targets email templates of Drupal 6 sites and

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new888 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_5.patch.
[ View ]
new8.45 KB
PASSED: [[SimpleTest]]: [MySQL] 32,023 pass(es).
[ View ]

The only change to #20 is that I added a comment for variable_get because it baffled me why do we do that (and removed a not needed change in simpletest.info). Interdiff is attached too. Otherwise the code is nice and has a test.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, interdiff.patch, failed testing.

Status:Needs work» Reviewed & tested by the community

Shut up, bot. :)

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new8.45 KB
PASSED: [[SimpleTest]]: [MySQL] 32,022 pass(es).
[ View ]
new484 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_6.patch.
[ View ]

The only change to #23 is that I incorporated #22.

Status:Needs review» Reviewed & tested by the community

I did a trivial one character fix in the doc block. I'm assuming chx and everyone else will be happy to have this back in the RTBC queue right away after it has passed the bot.

Status:Reviewed & tested by the community» Needs work

Hm. It says this no longer applies, I guess because of #761648: Menu D6->D7 upgrade doesn't maintain node-menu configuration (f.k.a.: Menu items disappear after upgrade or manual menu entry)?

Could we get a super-fast re-roll here, please? :)

Status:Needs work» Needs review
StatusFileSize
new8.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_reroll.patch.
[ View ]

Quick re-roll, not tested.

Status:Needs review» Needs work

The last submitted patch, user_reroll.patch, failed testing.

Status:Needs work» Reviewed & tested by the community

I only reviewed the code, but if #23 was RTBC then so is #29 (assuming it comes back green).

Status:Reviewed & tested by the community» Needs work

Oops, crosspost with the bot. :(

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new8.56 KB
PASSED: [[SimpleTest]]: [MySQL] 32,062 pass(es).
[ View ]

Anoter reroll.

Please give the credit of this to people that deserve it :)

Status:Reviewed & tested by the community» Fixed

Awesome, thanks folks!! :D Fix looks great, tests are very thorough.

Committed to 7.x. Another critical bites the dust!

Status:Fixed» Closed (fixed)
Issue tags:-D7 upgrade path

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