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)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Eric_A’s picture

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

Eric_A’s picture

mail@victorquinn.com’s picture

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.

Eric_A’s picture

Issue tags: +Needs tests

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

Eric_A’s picture

Priority: Normal » Major
FileSize
4.82 KB
2.44 KB

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

bfroehle’s picture

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

Eric_A’s picture

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

Eric_A’s picture

Title: User email template tokens not upgraded » User 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.

Eric_A’s picture

Title: User email template tokens not upgraded properly » User email template tokens not upgraded
FileSize
7.28 KB
4.9 KB

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

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

Eric_A’s picture

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

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.

Eric_A’s picture

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

Eric_A’s picture

Title: User email template tokens not upgraded » user_update_7011() completely broken (User email template tokens not upgraded)
FileSize
6.77 KB
4.38 KB

Removed t() from assert message.

Eric_A’s picture

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

Uccio’s picture

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

Starminder’s picture

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

catch’s picture

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.

jeffschuler’s picture

yikes. subscribing.

Eric_A’s picture

Status: Needs work » Needs review
FileSize
6.92 KB
4.53 KB

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?

catch’s picture

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.

Eric_A’s picture

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

Eric_A’s picture

+ * 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

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
888 bytes
8.45 KB

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.

webchick’s picture

Status: Needs work » Reviewed & tested by the community

Shut up, bot. :)

Eric_A’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
8.45 KB
484 bytes

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

Eric_A’s picture

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.

webchick’s picture

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

catch’s picture

Status: Needs work » Needs review
FileSize
8.45 KB

Quick re-roll, not tested.

Status: Needs review » Needs work

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

tstoeckler’s picture

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

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

Oops, crosspost with the bot. :(

aspilicious’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
8.56 KB

Anoter reroll.

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

webchick’s picture

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.