From: Robert L M. <li...@ti...> - 2006-07-27 18:05:34
|
While testing the development version of SquirrelMail (1.5.1), I've noticed an odd problem. After sending many replies during one session, SquirrelMail (PHP?) would eventually stop correctly handling the HTTP posts back to compose.php when the messages was sent or saved as a draft. In other words, I'd: 1. Login via an https connection; 2. View an incoming message (doesn't matter which); 3. Press "Reply"; 4. Press "Send". 5. Repeat steps 2-4 lots of times without logging out. This worked normally at first, but after sending perhaps a dozen messages, I would be shown a completely empty compose screen (all text boxes empty) after step 3, as if I had just started composing a brand new message -- and the message would not be sent. Once this started happening, it would keep happening every time I composed a message until I logged out and in again (which fixed it). I believe I've narrowed down the cause, although perhaps I'm just misunderstanding something. What seems to be happening is as follows: SquirrelMail keeps track of messages that are in the process of being composed, using the variable $compose_messages in compose.php. This variable is stored in the session, using: sqsession_register($compose_messages, 'compose_messages'); ... and is also passed around as a hidden field in the HTML compose form ("restoremessages"). Now, when a composed message is successfully sent (or a draft is saved), the code does this: unset($compose_messages[$session]); That is presumably intended to make it "forget" about that particular message being composed. However, that change is never saved to the session -- that is, there is no subsequent call to sqsession_register; the "unset" change gets forgotten. Because of that, the next time you compose a message, the old entry is still present in the session. So $compose_messages grows quite large after sending several messages, and -- here's the weird part -- it eventually gets so big that when it's passed around using the HTML form's hidden "restoremessages" field, it somehow breaks the subsequent HTTP post data (see below). Am I correct in thinking that after unsetting $compose_messages[$session], the code should use sqsession_register to permanently save that change? It seems to solve the problem for my tests. If that's the right fix, here's a trivial patch against CVS head: --- compose.php.orig 2006-07-25 13:32:59.000000000 -0700 +++ compose.php 2006-07-25 13:37:40.000000000 -0700 @@ -373,6 +373,7 @@ exit(); } else { unset($compose_messages[$session]); + sqsession_register($compose_messages, 'compose_messages'); $draft_message = _("Draft Email Saved"); /* If this is a resumed draft, then delete the original */ if(isset($delete_draft)) { @@ -472,6 +473,7 @@ exit(); } unset($compose_messages[$session]); + sqsession_register($compose_messages, 'compose_messages'); /* if it is resumed draft, delete draft message */ if ( isset($delete_draft)) { -------------------------------------------------------------------- I'm still puzzled about the fact that the hidden field growing so large can break the HTTP post in the first place, though. What's even weirder is that it only breaks when the post is submitted via SSL ("https://"); if I access SquirrelMail via "http://", it doesn't happen. Yeah, I know: that sounds like nonsense, but I've spent huge amounts of time testing it very carefully, using two different browsers (Konqueror and Firefox). It's completely repeatable via https links and never happened to me with http links. At first I suspected a browser bug, but since it happens on both browsers.... I'm using SquirrelMail 1.5.1, PHP 4.3.10 (Debian stable version) in CGI mode, and Apache 2.0.54 (again, Debian version). If anyone knows the underlying cause, I'd very much like to hear it. Even with the above patch, you can still make the hidden "restoremessages" field get huge by starting to reply to many messages but not sending them or saving them as a draft, so the general issue is still potentially a problem. -- Robert L Mathews "The trouble with doing something right the first time is that nobody appreciates how difficult it was." |
From: Michael A. <blu...@ya...> - 2007-06-27 01:22:28
|
Robert L Mathews wrote: > > Even with the above patch, you can still make the hidden "restoremessages" > field get huge by starting to reply to many messages but not sending them > or saving them as a draft, so the general issue is still potentially a > problem. > Sorry to bump an old thread, but I think this is a real issue. With a fresh session, compose.php loads at ~9 KB. Just click compose over and over again, and you'll see the size increasing ~2 KB at every new request. The hidden "restoremessages" field would get more bulked with every composed-but-not-sent action. We're talking a possible ~200 KB (or even more) for a compose screen here, which I think is a huge number for low-bandwidth users. I don't know, it just doesn't feel right (or good practice) to put such data in an input element (though I got no better alternatives than the current method, sorry). Has this been fixed in the latest devel build or is there a workaround? -- View this message in context: http://www.nabble.com/%24compose_messages-not-saved-in-session-after-being-unset--%281.5.x%29-tf2013337.html#a11316781 Sent from the squirrelmail-devel mailing list archive at Nabble.com. |
From: Jonathan A. <jo...@sq...> - 2007-07-08 06:06:56
|
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Michael Andreas wrote: > > Robert L Mathews wrote: >> Even with the above patch, you can still make the hidden "restoremessages" >> field get huge by starting to reply to many messages but not sending them >> or saving them as a draft, so the general issue is still potentially a >> problem. >> > > Sorry to bump an old thread, but I think this is a real issue. With a fresh > session, compose.php loads at ~9 KB. Just click compose over and over again, > and you'll see the size increasing ~2 KB at every new request. The hidden > "restoremessages" field would get more bulked with every > composed-but-not-sent action. We're talking a possible ~200 KB (or even > more) for a compose screen here, which I think is a huge number for > low-bandwidth users. I don't know, it just doesn't feel right (or good > practice) to put such data in an input element (though I got no better > alternatives than the current method, sorry). Wow... you made me dig for the original one :) Looking at stable, it's not been fixed there, nor has it been fixed in devel either. What's weird about this feature is that we make each compose message unique by assigning it a new ID, but give the user no method of getting back to an old compose in the event they may have accidentally hit close (yes I know of the auto save plugin). Thanks for bringing this one back up, code fixed. - -- Jon Angliss <jo...@sq...> -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.7 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFGkH74K4PoFPj9H3MRApVrAJ9V23PLsxFoPSFiZ3hnTxfwgxIsJACg0KNT 8lOCPJcIoK7ptZhHSqnOFjg= =x7Ez -----END PGP SIGNATURE----- |
From: Paul L. <pa...@sq...> - 2007-07-08 08:01:37
|
On 7/7/07, Jonathan Angliss <jo...@sq...> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Michael Andreas wrote: > > > > Robert L Mathews wrote: > >> Even with the above patch, you can still make the hidden "restoremessages" > >> field get huge by starting to reply to many messages but not sending them > >> or saving them as a draft, so the general issue is still potentially a > >> problem. > >> > > > > Sorry to bump an old thread, but I think this is a real issue. With a fresh > > session, compose.php loads at ~9 KB. Just click compose over and over again, > > and you'll see the size increasing ~2 KB at every new request. The hidden > > "restoremessages" field would get more bulked with every > > composed-but-not-sent action. We're talking a possible ~200 KB (or even > > more) for a compose screen here, which I think is a huge number for > > low-bandwidth users. I don't know, it just doesn't feel right (or good > > practice) to put such data in an input element (though I got no better > > alternatives than the current method, sorry). > > Wow... you made me dig for the original one :) Looking at stable, it's > not been fixed there, nor has it been fixed in devel either. > > What's weird about this feature is that we make each compose message > unique by assigning it a new ID, but give the user no method of getting > back to an old compose in the event they may have accidentally hit close > (yes I know of the auto save plugin). That is, "quicksave". Quicksave saves message text in real time (in a cookie, which naturally has size limitations, and in the future via an AJAX interface, which won't have size limitations), which is different than a message that is actually submitted to the server and stored in session. However, it might be useful to try to make use of that information. Dunno if Quicksave can, but maybe I'll try to have a look. > Thanks for bringing this one back up, code fixed. |
From: Paul L. <pa...@sq...> - 2007-07-08 10:04:20
|
On 7/8/07, Paul Lesniewski <pa...@sq...> wrote: > On 7/7/07, Jonathan Angliss <jo...@sq...> wrote: > > -----BEGIN PGP SIGNED MESSAGE----- > > Hash: SHA1 > > > > Michael Andreas wrote: > > > > > > Robert L Mathews wrote: > > >> Even with the above patch, you can still make the hidden "restoremessages" > > >> field get huge by starting to reply to many messages but not sending them > > >> or saving them as a draft, so the general issue is still potentially a > > >> problem. > > >> > > > > > > Sorry to bump an old thread, but I think this is a real issue. With a fresh > > > session, compose.php loads at ~9 KB. Just click compose over and over again, > > > and you'll see the size increasing ~2 KB at every new request. The hidden > > > "restoremessages" field would get more bulked with every > > > composed-but-not-sent action. We're talking a possible ~200 KB (or even > > > more) for a compose screen here, which I think is a huge number for > > > low-bandwidth users. I don't know, it just doesn't feel right (or good > > > practice) to put such data in an input element (though I got no better > > > alternatives than the current method, sorry). > > > > Wow... you made me dig for the original one :) Looking at stable, it's > > not been fixed there, nor has it been fixed in devel either. > > > > What's weird about this feature is that we make each compose message > > unique by assigning it a new ID, but give the user no method of getting > > back to an old compose in the event they may have accidentally hit close > > (yes I know of the auto save plugin). > > That is, "quicksave". Quicksave saves message text in real time (in a > cookie, which naturally has size limitations, and in the future via an > AJAX interface, which won't have size limitations), which is different > than a message that is actually submitted to the server and stored in > session. However, it might be useful to try to make use of that > information. Dunno if Quicksave can, but maybe I'll try to have a > look. After looking at this a bit, I'm not sure I see the use of $compose_messages at all. Supposedly it is placed in the form as a hidden input so after a session timeout on the compose page, the whole thing can be restored, BUT what does SM ever do with them anyway? Did someone a long time ago think some day we'd have a screen where the user could look at a list of "lost" messages that were for some reason never sent? Otherwise, I can't think of anywhere this information would be used. A grep shows the data isn't used anywhere for that sort of thing -- compose.php looks like it can accept a $composesession input to recall one of them, but I don't see anywhere that we use it...? Unless I am missing something (please correct me), I think it should be removed. This would help clean out $_SESSION as well as keep the compose page more lightweight. The usefulness of recovering any messages stored in the $_SESSION is very low because the user has to click submit to get the message into the session, and usually the message is lost because the Send button is never pressed. In the more rare case that Send is clicked but the message is not sent, the compose screen reloads with the message again, so it is not lost anyway. Sure, the user could then click away (e.g., back to the INBOX) without sending, so that message is saved in session, but there is not any way to restart that message in SM, and I'm not sure it adds much value to create a mechanism to retrieve it, especially when the Quicksave plugin is probably good enough for that. In the case of a session timeout on the compose screen, the only data you really need to restore is the message that was being composed (not older ones), and that data will be in the standard message POST fields, so we don't need any hidden compose_messages data at all. Thoughts? I think we should clear out this code, at least in 1.5.2. - Paul |
From: Michael A. <blu...@ya...> - 2007-07-09 01:44:54
|
Jonathan Angliss wrote: > > Thanks for bringing this one back up, code fixed. > Paul Lesniewski wrote: > > Thoughts? I think we should clear out this code, at least in 1.5.2. > Hi Paul and Jon, thank you for looking into the code. For what it's worth, I have stripped out the relevant code on my SquirrelMail installation and use it for two weeks without any problems whatsoever. So I definitely second the clear out. -- View this message in context: http://www.nabble.com/%24compose_messages-not-saved-in-session-after-being-unset--%281.5.x%29-tf2013337.html#a11494795 Sent from the squirrelmail-devel mailing list archive at Nabble.com. |
From: Jonathan A. <jo...@sq...> - 2007-07-09 01:56:00
|
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Paul Lesniewski wrote: > On 7/8/07, Paul Lesniewski <pa...@sq...> wrote: >> On 7/7/07, Jonathan Angliss <jo...@sq...> wrote: >>> -----BEGIN PGP SIGNED MESSAGE----- >>> Hash: SHA1 >>> >>> Michael Andreas wrote: >>>> Robert L Mathews wrote: >>>>> Even with the above patch, you can still make the hidden "restoremessages" >>>>> field get huge by starting to reply to many messages but not sending them >>>>> or saving them as a draft, so the general issue is still potentially a >>>>> problem. >>>>> >>>> Sorry to bump an old thread, but I think this is a real issue. With a fresh >>>> session, compose.php loads at ~9 KB. Just click compose over and over again, >>>> and you'll see the size increasing ~2 KB at every new request. The hidden >>>> "restoremessages" field would get more bulked with every >>>> composed-but-not-sent action. We're talking a possible ~200 KB (or even >>>> more) for a compose screen here, which I think is a huge number for >>>> low-bandwidth users. I don't know, it just doesn't feel right (or good >>>> practice) to put such data in an input element (though I got no better >>>> alternatives than the current method, sorry). >>> Wow... you made me dig for the original one :) Looking at stable, it's >>> not been fixed there, nor has it been fixed in devel either. >>> >>> What's weird about this feature is that we make each compose message >>> unique by assigning it a new ID, but give the user no method of getting >>> back to an old compose in the event they may have accidentally hit close >>> (yes I know of the auto save plugin). >> That is, "quicksave". Quicksave saves message text in real time (in a >> cookie, which naturally has size limitations, and in the future via an >> AJAX interface, which won't have size limitations), which is different >> than a message that is actually submitted to the server and stored in >> session. However, it might be useful to try to make use of that >> information. Dunno if Quicksave can, but maybe I'll try to have a >> look. > > After looking at this a bit, I'm not sure I see the use of > $compose_messages at all. Supposedly it is placed in the form as a > hidden input so after a session timeout on the compose page, the whole > thing can be restored, BUT what does SM ever do with them anyway? When I saw it, it had me baffled too. I cannot really think of any reason for it being there, short of somebody adding the ability to go back and resume a compose, BUT... for that to be effective, you'd have already had to have hit the send, so it'll either be a) sent, or b) in your drafts folder. The only exception is if you told it to add your signature, and then 'closed' the compose page, but the quicksave plugin would handle that instance I believe. > Unless I am missing something (please correct me), I think it should > be removed. This would help clean out $_SESSION as well as keep the > compose page more lightweight. I don't think so. It was probably a feature that was planned, but never added entirely. I'd have to go through the CVS logs to see if we can dig anything out, but I'm not sure it's worth the effort. > Thoughts? I think we should clear out this code, at least in 1.5.2. I vote for stripping it out in dev, and see where it takes us. I doubt it'll have any negative impact. - -- Jon Angliss <jo...@sq...> -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.7 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFGkZWlK4PoFPj9H3MRAnwfAJ4qx54u/2ArEK20UkJaL+AyCmoILACgza9j dMkSmRlX4lUfyT4bxs6y1Ug= =lgAi -----END PGP SIGNATURE----- |
From: Thijs K. <ki...@sq...> - 2007-07-09 07:59:28
|
On Monday 9 July 2007 03:55, Jonathan Angliss wrote: > > After looking at this a bit, I'm not sure I see the use of > > $compose_messages at all. Supposedly it is placed in the form as a > > hidden input so after a session timeout on the compose page, the whole > > thing can be restored, BUT what does SM ever do with them anyway? > > When I saw it, it had me baffled too. I cannot really think of any > reason for it being there, short of somebody adding the ability to go > back and resume a compose, BUT... for that to be effective, you'd have > already had to have hit the send, so it'll either be a) sent, or b) in > your drafts folder.=20 This is used for, and works, when your session times out. It works like this: 1) You write a long mail; 2) Meanwhile your session times out; 3) You hit send; 4) You are confronted with a "You must be logged in..." message; 5) You log in, and the compose message is restored. This works just fine right now. What misses is in my opinion a different er= ror=20 message, so not "You must be logged in..." but "Your session timed out,=20 please relogin to continue to compose your message.". Thijs |
From: Paul L. <pa...@sq...> - 2007-07-10 05:17:44
|
On 7/9/07, Thijs Kinkhorst <ki...@sq...> wrote: > On Monday 9 July 2007 03:55, Jonathan Angliss wrote: > > > After looking at this a bit, I'm not sure I see the use of > > > $compose_messages at all. Supposedly it is placed in the form as a > > > hidden input so after a session timeout on the compose page, the whole > > > thing can be restored, BUT what does SM ever do with them anyway? > > > > When I saw it, it had me baffled too. I cannot really think of any > > reason for it being there, short of somebody adding the ability to go > > back and resume a compose, BUT... for that to be effective, you'd have > > already had to have hit the send, so it'll either be a) sent, or b) in > > your drafts folder. > > This is used for, and works, when your session times out. > > It works like this: > > 1) You write a long mail; > 2) Meanwhile your session times out; > 3) You hit send; > 4) You are confronted with a "You must be logged in..." message; > 5) You log in, and the compose message is restored. Yes, but compose_messages contains a full history of unsent messages, not just the current one. There is no need that I can see to keep it both in session and placed in a hidden input field. The functionality you describe should work with the visible input elements on the compose page - no need for compose_messages or any hidden inputs related to it that I can see. Thijs, if you can confirm that I am not wrong, I can take responsibility for stripping it from DEVEL (and I will make sure the compose resume functionality still works). > This works just fine right now. What misses is in my opinion a different error > message, so not "You must be logged in..." but "Your session timed out, > please relogin to continue to compose your message.". That would be nice too. |
From: Paul L. <pa...@sq...> - 2007-08-27 07:43:22
|
First NOTE -- I am cross-posting to the SM-CVS list because commits don't seem to be going to the list today. Grrrr. Maybe they'll come thru in a batch later, but in case not, I am including the links to my commits today below along with two I found from Jon. On 7/9/07, Paul Lesniewski <pa...@sq...> wrote: > On 7/9/07, Thijs Kinkhorst <ki...@sq...> wrote: > > On Monday 9 July 2007 03:55, Jonathan Angliss wrote: > > > > After looking at this a bit, I'm not sure I see the use of > > > > $compose_messages at all. Supposedly it is placed in the form as a > > > > hidden input so after a session timeout on the compose page, the whole > > > > thing can be restored, BUT what does SM ever do with them anyway? > > > > > > When I saw it, it had me baffled too. I cannot really think of any > > > reason for it being there, short of somebody adding the ability to go > > > back and resume a compose, BUT... for that to be effective, you'd have > > > already had to have hit the send, so it'll either be a) sent, or b) in > > > your drafts folder. > > > > This is used for, and works, when your session times out. > > > > It works like this: > > > > 1) You write a long mail; > > 2) Meanwhile your session times out; > > 3) You hit send; > > 4) You are confronted with a "You must be logged in..." message; > > 5) You log in, and the compose message is restored. > > Yes, but compose_messages contains a full history of unsent messages, > not just the current one. There is no need that I can see to keep it > both in session and placed in a hidden input field. The functionality > you describe should work with the visible input elements on the > compose page - no need for compose_messages or any hidden inputs > related to it that I can see. Thijs, if you can confirm that I am not > wrong, I can take responsibility for stripping it from DEVEL (and I > will make sure the compose resume functionality still works). I have fixed this, along with making SM recover attachments now too, which is nice. PLEASE PLEASE PLEASE HELP TEST! I have tested a bunch, but this change deserves more. Please send messages. Send some with attachments. Save drafts. Resume and send drafts. Resume a draft, save it as a draft again, then resume and send it. Wipe out your session file before you click send on a message with and without an attachment, etc. PLEASE PLEASE TEST! > > This works just fine right now. What misses is in my opinion a different error > > message, so not "You must be logged in..." but "Your session timed out, > > please relogin to continue to compose your message.". > > That would be nice too. Done. Here are the diffs I commited for this (and a few below that unrelated and from Jon): http://squirrelmail.svn.sourceforge.net/viewvc/squirrelmail/branches/SM-1_4-STABLE/squirrelmail/ChangeLog?r1=12602&r2=12604 http://squirrelmail.svn.sourceforge.net/viewvc/squirrelmail/branches/SM-1_4-STABLE/squirrelmail/src/compose.php?r1=12542&r2=12604 http://squirrelmail.svn.sourceforge.net/viewvc/squirrelmail/branches/SM-1_4-STABLE/squirrelmail/functions/auth.php?r1=12546&r2=12604 In these I did some very minor stuff on the spamcop plugin: http://squirrelmail.svn.sourceforge.net/viewvc/squirrelmail/trunk/squirrelmail/plugins/spamcop/spamcop.php?r1=12128&r2=12603 http://squirrelmail.svn.sourceforge.net/viewvc/squirrelmail/branches/SM-1_4-STABLE/squirrelmail/plugins/spamcop/options.php?r1=10633&r2=12600 http://squirrelmail.svn.sourceforge.net/viewvc/squirrelmail/branches/SM-1_4-STABLE/squirrelmail/plugins/spamcop/spamcop.php?r1=11393&r2=12601 And Jon fixed a bug in a theme: http://squirrelmail.svn.sourceforge.net/viewvc/squirrelmail/branches/SM-1_4-STABLE/squirrelmail/themes/darkness.php?r1=12538&r2=12602 http://squirrelmail.svn.sourceforge.net/viewvc/squirrelmail/branches/SM-1_4-STABLE/squirrelmail/ChangeLog?r1=12597&r2=12602 |
From: Thijs K. <ki...@sq...> - 2007-07-10 08:28:59
|
On Tuesday 10 July 2007 07:17, Paul Lesniewski wrote: > Yes, but compose_messages contains a full history of unsent messages, > not just the current one. There is no need that I can see to keep it > both in session and placed in a hidden input field. The functionality > you describe should work with the visible input elements on the > compose page - no need for compose_messages or any hidden inputs > related to it that I can see. Thijs, if you can confirm that I am not > wrong, I can take responsibility for stripping it from DEVEL (and I > will make sure the compose resume functionality still works). Ah, I missed the detail of "contains all previous messages". That should be= =20 fixed. What the essential part of that code is, is to be able to store all message= =20 data that is not in the form itself. Currently that's only the attachments,= =20 but those are quite crucial to any composed message. You could consider to= =20 rewrite the code to only include the current message, or even only the=20 attachment part of that. Thijs |
From: Paul L. <pa...@sq...> - 2007-07-10 08:57:39
|
On 7/10/07, Thijs Kinkhorst <ki...@sq...> wrote: > On Tuesday 10 July 2007 07:17, Paul Lesniewski wrote: > > Yes, but compose_messages contains a full history of unsent messages, > > not just the current one. There is no need that I can see to keep it > > both in session and placed in a hidden input field. The functionality > > you describe should work with the visible input elements on the > > compose page - no need for compose_messages or any hidden inputs > > related to it that I can see. Thijs, if you can confirm that I am not > > wrong, I can take responsibility for stripping it from DEVEL (and I > > will make sure the compose resume functionality still works). > > Ah, I missed the detail of "contains all previous messages". That should be > fixed. > > What the essential part of that code is, is to be able to store all message > data that is not in the form itself. Currently that's only the attachments, > but those are quite crucial to any composed message. You could consider to > rewrite the code to only include the current message, or even only the > attachment part of that. OK. I haven't looked very deeply at where the attachment info is coming from - if from the compose_messages mothball, then yes, it will need to be routed out and preserved in a hidden input. Should have been that way anyway IMO. I'll do this some time tomorrow. Cheers, Paul |
From: Erin S. <ebu...@gm...> - 2007-07-12 03:35:45
|
On 7/10/07, Paul Lesniewski <pa...@sq...> wrote: > On 7/10/07, Thijs Kinkhorst <ki...@sq...> wrote: > > Ah, I missed the detail of "contains all previous messages". That should be > > fixed. > > > > What the essential part of that code is, is to be able to store all message > > data that is not in the form itself. Currently that's only the attachments, > > but those are quite crucial to any composed message. You could consider to > > rewrite the code to only include the current message, or even only the > > attachment part of that. > > OK. I haven't looked very deeply at where the attachment info is > coming from - if from the compose_messages mothball, then yes, it will > need to be routed out and preserved in a hidden input. Should have > been that way anyway IMO. I'll do this some time tomorrow. > > Cheers, > > Paul IIRC, there was a big stink a long time ago about compose_messages dropping information when the session expired, which is where the "mothball" came from. Saving only the "current" message implies that people only do one thing at a time. As 'Compose in new window' is a valid option, it's conceivable that multiple messages could be in progress when the session expires. If we're going to take out the compose_messages mothball, then it seems like quicksave should be brought into core. If there's one thing that gets people riled, it's losing their work because of some obscure thing called a "session timeout." Erin -- 'Waste of a good apple' -Samwise Gamgee |
From: Paul L. <pa...@sq...> - 2007-07-12 05:24:26
|
On 7/11/07, Erin Schnabel <ebu...@gm...> wrote: > On 7/10/07, Paul Lesniewski <pa...@sq...> wrote: > > On 7/10/07, Thijs Kinkhorst <ki...@sq...> wrote: > > > Ah, I missed the detail of "contains all previous messages". That should be > > > fixed. > > > > > > What the essential part of that code is, is to be able to store all message > > > data that is not in the form itself. Currently that's only the attachments, > > > but those are quite crucial to any composed message. You could consider to > > > rewrite the code to only include the current message, or even only the > > > attachment part of that. > > > > OK. I haven't looked very deeply at where the attachment info is > > coming from - if from the compose_messages mothball, then yes, it will > > need to be routed out and preserved in a hidden input. Should have > > been that way anyway IMO. I'll do this some time tomorrow. > > > > Cheers, > > > > Paul > > > IIRC, there was a big stink a long time ago about compose_messages > dropping information when the session expired, which is where the > "mothball" came from. Saving only the "current" message implies that > people only do one thing at a time. As 'Compose in new window' is a > valid option, it's conceivable that multiple messages could be in > progress when the session expires. Yes, or even in multiple tabs. However, once one of them is revived via the restoration mechanism, the others will work just fine again, unless I'm not thinking straight. So we still don't need all those "sessions"... and moreover, as we've discussed, the only things in those "sessions" are messages that have actually been submitted once, which is not going to be the most common case. If you have two or three concurrent compose windows open, chances are that none of them will be saved in the PHP session when you lose your session, even with the code the way it is now -- so it's pretty useless IMO. > If we're going to take out the compose_messages mothball, then it > seems like quicksave should be brought into core. If there's one thing > that gets people riled, it's losing their work because of some obscure > thing called a "session timeout." I dunno about that. I mean, sure, I'm fine pulling in quicksave, but this poor-man's restore functionality is about as useful as, ummm, well, whatever the right phrase is... it's NOT useful. It's just NOT possible for SM core to retrieve messages unless they've been submitted once already, and the housecleaning I am working on will not result in any loss or change of functionality. It will still be just as useless. ;-) BTW, it's more involved than I thought - used by the message list page to build forward messages and whatnot -- and compose.php is actually the epitome of "spaghetti code" so I'm picking through it trying to see what the best approach is. For STABLE, it's too involved to pull it all out without feeling like it could be an UNstable change, so for there I am going to leave it alone, but just make sure that it is always empty. Cheers, Paul |
From: Thijs K. <ki...@sq...> - 2007-07-14 19:23:23
|
On Thursday 12 July 2007 07:24, Paul Lesniewski wrote: > If you have two or > three concurrent compose windows open, chances are that none of them > will be saved in the PHP session when you lose your session, even with > the code the way it is now -- so it's pretty useless IMO. They are saved - the form fields are still filled in, so you can either (1) relogin in a different tab and submit your mail, or (2) press Send, receive the "You must be..." message, and use the restore functionality to continue composing your message. What does not seem to work is the attachment handling. So removing that code entirely shouldn't hurt, but adding new code to actually fix the attachment restoration would be nice. Thijs |
From: Paul L. <pa...@sq...> - 2007-07-14 20:01:19
|
On 7/14/07, Thijs Kinkhorst <ki...@sq...> wrote: > On Thursday 12 July 2007 07:24, Paul Lesniewski wrote: > > If you have two or > > three concurrent compose windows open, chances are that none of them > > will be saved in the PHP session when you lose your session, even with > > the code the way it is now -- so it's pretty useless IMO. > > They are saved No, the point is that they are *probably* NOT saved, because to save them with the mechanism SM provides, you need to have already submitted the form once. If you happen to have done that, sure, the info is saved, HOWEVER, removing the info from the "mothball" does no harm, because the most current state of those fields is still in the form on the client, so when it is next submitted - whether or not the session has expired, or expired and resumed - the information will be taken by the server and everything is fine again. > - the form fields are still filled in, so you can either (1) > relogin in a different tab and submit your mail, or (2) press Send, receive > the "You must be..." message, and use the restore functionality to continue > composing your message. Oh right, well, we are both saying the exact same thing. ;-) Point is that relieving ourselves of the compose_messages mess hurts nothing and lightens the hidden fields and session data. > What does not seem to work is the attachment handling. So removing that code > entirely shouldn't hurt, but adding new code to actually fix the attachment > restoration would be nice. Yeah, my plan is to definitely make sure the attachments are still presented in a hidden field. I hope I can get this together this weekend! Cheers, Paul |