Reset user mail variables.

I am not sure how to achieve thuis in drupal,s update.php, but dpkg-reconfigure or apt-get upgrade have a nice method:

If a config file was found altered by the admin/user, apt or dpkg comes up with a message something alike:
- the new config file /etc/foobar.conf differs from the one edited by you,
Do you want to view a diff (D), overwrite (Y) ignore and keep the old one (N) skip and come back later (S) [s/d/y/N]

Maybe we should simply prompt the user with a message that the welcome message was changed, and that he/she chan choose to:
( ) keep the old message
( ) replace with the new message

But again: I have no clue how update.php coould handle that.

Ber

I think a notice or a warning in the update page would do, then the admin only has to delete the text on the admin page and he will get the new one.

I wouldn't like to delete anybody's text without warning and this one can be a long message.

if we go with a warning in update.php, then it will also need a mention in the upgrade.txt file. update.php is not something that is not easly refernced when preparing for an update until yourun it. While people should run it on a test site, it is something that can be missed.

We need to do something. There is no ideal solution as we can't transform one's data. I think we should reset the message so it's using the default message again. This will make sure it works for everyone but requires the user to reconfigure his/her customized mail. We can mention this in the release notes.

what about a

$old = variable_get('some_user_mail_text', xxx);
variable_set('some_user_mail_text_4.6', $old);
variable_set('some_user_mail_text', yyy);

and then

//the mail settings forms:
$description = t('some help');
$description .= t('<hr />Originally you had:</br>');
$description .=  variable_get('some_user_mail_text_4.6', $old);

If you like this idea, Dries, I will create a patch today.

Or, in human language: we store the mailsettings in a separate variable, then print that under the form in the help text.

Since we have no proper way to unset variables (Or not that i know of) we will add some cruft to the DB, but that is better then losing carefully crafted letters.

I, for one, have some sites where my clients spent a lot of effort crafting these mails. They will be quite angry to learn they have to re-do that, because I upgraded their sites. And copypasting this all manual, is not ideal either.

@ber - see variable_delete() ... also, i suggest removing the markup from t() in that message

How much time can you spend crafting such a message?

I wouldn't pollute the interface/code with legacy stuff. I would keep all of that in database/updates.inc. Can't we write the old data to the watchdog and be done with it? Or add a message to the query/upgrade report.

I vote for simply deleting the old message and writing a copy/notice to the watchdog table.

I am developing a small patch now. I try to keep the cruft as small as possible.
My idea is to delete all the legacy vars as soon as one stores the settings page.

Bèr

This is a patch. I tested it twice, but would really appreciate more tests.

I tried to keep it as small as possible. Also note that the function where the legacy stuff is called, is visited very infrequent, so the overhead for Drupal will only be measurable in extra bytes loaded into memory. In contrary to my previous plan, I decided to display this at the top as one big chunk. That saved a lot of if()s spread all over that function.

One small issue that I did not address is he display of endlines. I cannot call the filters, since we have no clue what filters a user may have. The result is that the messages aer displayed a bit ugly. One would need to add newlines him/herself again.

please test!

I wouldn't pollute the interface/code with legacy stuff. I would keep all of that in database/updates.inc.

I am with Dries on this.

Right, so how to deal with this then?

a) we cannot automatically convert users messages into new ones that work, or not that I am aware of.
b) A human is needed to convert humanly created messages to a new one.
c) without new messages, one of the primary features in the site will no longer work.
d) just deleting messages will anger people At least it would anger me. I would be very pissedoff to find that my carefully crafted welcome message suddenly is replaced by a default one. Esp on a busy site that can result in a lot of people not receiving proper information.

Untill now, this is the only patch, so I prefer to see alternatives or improvements. Untill now, this is the only solution that I am aware of that solves all the four issues.

oh, and notice that this patch only shows messages that were actually changed by a user. Others are not converted (no need to) and, thus not displayed.

This is how we deal with this.

I'd prefer to remove the variables from the variables table, resetting the password e-mail.
Documenting is not enough.

Hm. Not sure about just blanketly overwriting their welcome message is a good idea. :\ I know some client sites I've worked on they've had actual content writers make a unique welcome mail just for their site and if this was gone in a flash with no warning they would likely be quite peeved.

What if we did this? What if we did a check to see if the welcome e-mail variable either a) didn't exist or b) matched the default value that's in Drupal 4.6... if so, then just blindly over-write. Else, warn them that they need to replace %login_uri with %login_url in their welcome message as part of the update process.

Let's write the old one to the watchdog and to the screen, or something.

I haven't been active for a long time here, i hope i will say smart things :-)

I spent some time looking at this today. What i think would be clean, and nice, would be to display "warnings" at the end of update process.

I need a closer look, but this could be fairly easy to do by adding a parameter "warning" to update_sql,

function update_sql($sql, $warning = '') {
$result = db_query($sql);
return array('success' => $result !== FALSE, 'query' => check_plain($sql), 'warning' = $warning);
}

and display these warnings in the function update_finished_page().

If somebody is interested, i can try to propose a patch tomorrow morning.

Sorry

array('success' => $result !== FALSE, 'query' => check_plain($sql), 'warning' => $warning);

this is an exemple code, need more time to make work.

what webchick describes is what my patch does.

Dries, can you comment on that patch?

the patch at #10 still applies btw http://drupal.org/node/28868#comment-76100

Ber: I don't like it. I don't want legacy code in user.module.

we could send an email with the old text to the homepages' e-mail-address
so it got saved...

I am with killes.

-1 for CHXes approach.

1) the people receiving the messages about "you need to change this and that" are, in this case, the visitors of a site. We want this to get to the maintainers.

2) anything automatic will break, or will at least produce ugly messages. Note to all: there are a lot of people, sites and cases where these messages are part of the most important texts of the site! Webchick pointed to a case where writers where hired to create these messages. I have been in teams where we needed to hold of the release of the site for weeks (!) becuase those in charge did not like some specific mail messages.

I am fine with storing this in the watchdog, as long as there is some way to link to it. (and so far there is none that i can see, that involve no legacy code)

if we cannot get this solved. Can we please go for CHXes first approach of
* not changing anything.
* noting this in the upgrade docs.

that is by far the simplest, cleanest and safest one. Yes, people will not read the upgrade docs. Yes people will forget. But it is far better then deleting mails or replacing them with possible nonsense (aka preg-replaced)

This is probably not the last time something like this is going to come up. Dries and others do not want legacy code hanging around to handle it. The optimizer in me feels the same way.

Let's create a good, general solution that will work for future problems similar to this one.

Maybe we need to build something into the admin interface which will allow a site maintainer to look at old text, saved someplace in the database specifically for this purpose. This idea requires database capability and admin changes.

Or maybe it could just be an unpublished node with a system-generated title or something -- maybe even with an upgrade-generated comment saying what needs to be changed (e.g. %login_uri with %login_url). Issue a message to the effect of "view unpublished nodes with title XYZ for further information" and/or provide a link to the page and/or log a watchdog message. This ideas requires update create a node and attached comment.

Actually I like the second idea -- so please shoot some holes in it before I create a patch which will implement it. :-)

Please not insert junk into 50,000 node tables (approx number of drupal sites)? thats wors than legacy code IMO. variables table i can live with.

+ 1 for

Dries> Let's write the old one to the watchdog and to the screen

I think this this can be a good general solution. The site admin is warned and the information is kept somewhere. This should be enough.

Let's not make a big issue of this small thing...

Yes, let's proceed with my suggestion solution. Create a new watchdog category ('upgrade') if you wish but let's not complicated matters.

And perhaps a note at the end of the upgrade process (op=finished) that the admin should check his watchdog messages etc. etc. might also be prudent (and generic).

-K

Heads up. I am stuck, but ive got to go for a few hours.

This patch makes the watchdog entries fine.

But it acts weird on me in the output of $ret[]. I get this

<div id="update-results"><h2>The following queries were executed</h2><h3>system module</h3><h4>Update #175</h4><ul><li class="success"><</li><li class="success"><</li><li class="success"><</li><li class="success"><</li><li class="success"><</li><li class="success"><</li></ul></div>

Right, I am back, will work on this a bit more later on.
Has anyone got a clue why my patch does not produce a link to the watchdog entries, but only a '<' ?

BTW: another JS thing is that debugging becomes hard, since print_r '$ret' plain breaks :/ Minor and unrealted, but still.. :)

Bèr

I am off for the weekend. Please assign to yourself, if you wish to get this solved. I will be back on monday, and I will then assign to myself if still not fixed.

RErolled the patch. I found the issue with the odd links, it was the return array that was not properly mde in my patch.

This patch adds only code to update.inc.
* It stores the old messages in watchdog, only when they were changed.
* It resets the old messages into the new ones.
* It presents links to the watchdog entries on the update.php log page
* It stores the messages under the watchdog cat. 'legacy' (usefull for providing support)

Won't work with table prefixing.

oops. *blush*.

The update number is wrong.
Fixing it, doesn't seem to make things work.

rerolled.

Somehow my last cvs upgrade run amok on some funny file. This is against the lastest HEAD.

You will not see any messages if you have not modified your email messages. So to test, just edit your messages in your site *before* upgrading.

Can anyone suggest something better than: "Old %message_id is stored"? It doesn't sound quite right to me, but I might be mistaken. This patch is almost ready. Thanks for the persistence, Ber.

I changed the message to The mail template %message_id is reset to the default, and the old one is saved.. Better messages are welcome!

Good enough for me! Committed to HEAD.