From: Paul L. <pa...@sq...> - 2008-02-03 23:53:46
|
On Feb 3, 2008 3:46 PM, Paul Lesniewski <pa...@sq...> wrote: > > On Jan 31, 2008 7:28 AM, Thierry Godefroy <mel...@ya...> wrote: > > > > --- Paul Lesniewski <pa...@sq...> wrote: > > > > > On Jan 24, 2008 3:07 AM, Alexandros Vellis <av...@no...> wrote: > > > > > > > On Thu, 24 Jan 2008 11:45:44 +0100 Thijs Kinkhorst > > > > <ki...@sq...> wrote: > > > > > > > > > Hi Thierry, > > > > > > > > > > On Thursday 24 January 2008 11:07, Thierry Godefroy wrote: > > > > > > > > > > > As I am developing a plugin which relies on X-SMTP headers (X-P772 > > > > > > standard, to be more precise), I needed for a better support of > > > > > > these headers in Squirrelmail, which for now mostly ignores them. > > > > > > > > > > > > The result of this work is the attached tiny patch which: > > > > > > - adds X-P772-Primary-Precedence support (this is the X-P772 > > > > > > equivalent to X-Priority, Importance and Priority); > > > > > > - makes Rfc822Header.class store any X-SMTP header it parses in its > > > > > > more_headers array, instead of discarding them. > > > > > > > > > > Thank you for your patch. I'm a bit reserved about this one, because > > > > > it adds support for a standard which is specific to the French > > > > > military. It is only published in French and not through any official > > > > > standards body. I think this is really the kind of thing that should > > > > > > I agree > > > > > > > > be implemented in plugins as much as possible. I realise that parsing > > > > > headers is currently suboptimal from a plugin, but I'd rather see > > > > > that improved in general than to add such specific headers to our > > > > > code base... > > > > > > Right... > > > > > > > I had the exact same issue with an in-house patch of ours, targetted > > > > towards marking SPAM messages in folders. > > > > > > > > What it did was check for X-Spam-<foo> headers in messages inserted > > > > previously by sendmail, in a patch that is very similar to the one by > > > > Thierry Godefroy. Avelsieve's spam rule function inserted automatically > > > > a custom highlight rule that changed that messages' color, depending > > > > on that header. > > > > > > > > My point is that there is a need for things like my patch and Thierry's > > > > patch, to make this functionality accessible to plugins. > > > > Would anyone mind if I build the plugin architecture for this? I think > > > > it's something like three more hooks needed. Is it that terrible that we > > > > place such hooks in Rfc822Header.class.php? > > > > > > But the proposed patch here does little more than make sure that the > > > message as it is represented inside of SM contains the needed headers > > > as retrieved from IMAP. I think you might be trying to add more > > > functionality than Thierry is with this patch. I think the part of > > > his patch that should be added right away is this: > > > > > > diff -urN squirrelmail/class/mime/Rfc822Header.class.php > > > squirrelmail-melinda/class/mime/Rfc822Header.class.php > > > --- squirrelmail/class/mime/Rfc822Header.class.php 2008-01-15 > > > 16:34:17.000000000 +0100 > > > +++ squirrelmail-melinda/class/mime/Rfc822Header.class.php 2008-01-15 > > > 16:09:56.000000000 +0100 > > > @@ -353,6 +355,9 @@ > > > $this->x_spam_status = $this->parseSpamStatus($value); > > > break; > > > default: > > > + if (substr($field, 0, 2) == 'x-') { > > > + $this->more_headers[$field] = $value; > > > + } > > > break; > > > } > > > } > > > > > > This, of course, only adds headers with "X-...." format, so we could > > > change that. I don't see the need for a hook to do that. Adding more > > > hooks means impeding the speed of the core, so if we can avoid that, > > > we should. > > > > > > As for his proposed patch to functions/imap_messages.php, I do not see > > > why that is necessary, because he adds his header to > > > $aMsg['x-p772-primary-precedence'], but it *WILL* be added to $aMsg[$field] > > > (which might end up being the same thing?!?!). > > > > Not at all... OK, bear with me, because it's a tad bit intricate... :-P > > > > There are two ways the header fields are used: > > > > The first one is in mailbox_display.php, where the headers are used to decide > > what flags should be shown in the message list (and in the case of my plugin, > > what page the message subject link should point to: either the standard > > src/read_body.php for normal emails, or my plugin's > > plugins/melinda/read_body.php, for formal emails). > > mailbox_display.php uses $aMsg['x-priority'] to decide if the message should be > > flagged as urgent in the list, and $aMsg['x-priority'] was set by parseFetch() > > (in functions/imap_messages.php) after retrieving a _short_ list of headers > > with sqimap_get_small_header_list() (still in functions/imap_messages.php), > > among which the X-Priority, Priority and Importance fields. Equally, my plugin > > (which is called, _after_ the priority flag was set, thus the need for the > > patch adding x-p772-primary-precedence in parseFetch()), uses the other X-SMTP > > headers it made mailbox_display.php fetch (via the new 'fetch_more_headers' > > hook) to decide if the email subject link should be changed to point at its own > > version of read_body.php (thus the need for allowing to change this link via > > the 'subject_link' hook, again a small change part of my 'MelindaHooks' patch). > > During the mailbox displaying process, all the messages got their headers > > fetched from the IMAP server, thus the need for fetching only a minimal amount > > of headers for each message (and indeed, in its latest version, my plugin now > > only fetches two extra X-P772 headers when compared to the normal version of > > SQM). > > > > The second use of the headers is for displaying the messages (via > > read_body.php) and composing replies or forwarding them (via compose.php): here > > we only fetch the headers of one message, but we do need to fetch them all > > (thus the need for the X-SMTP headers patch in > > class/mime/Rfc822Header.class.php). > > > > I hope this clears it all... :-) > > Well, no, this does not explain why my comment above is wrong. You > introduced some different patches in this email, but for now, I am > addressing the ones you started this thread with. First, this one, > which I think is not needed: > > diff -urN squirrelmail/functions/imap_messages.php > squirrelmail-melinda/functions/imap_messages.php > --- squirrelmail/functions/imap_messages.php 2008-01-15 16:34:31.000000000 +0100 > +++ squirrelmail-melinda/functions/imap_messages.php 2008-01-18 > 11:09:20.000000000 +0100 > @@ -700,6 +700,8 @@ > $aMsg['date'] = > trim(str_replace(' ', ' ', $value)); > break; > case 'x-priority': > $aMsg['x-priority'] = ($value) ? (int) $value{0} : 3; break; > + case 'x-p772-primary-precedence': > + > $aMsg['x-p772-primary-precedence'] = $value; > case 'priority': > case 'importance': > // duplicate code with > Rfc822Header.cls:parsePriority() > > This is not needed because the default case just below looks like this: > > default: > $aMsg[$field] = $value; > break; > > So in the end, $aMsg has the very same value added to it. Now what I > am seeing that may be key to your patch is that > x-p772-primary-precedence does not have a "break;" so it also triggers > the execution of the code block that sets the priority > ($aMsg['x-priority']). Are you claiming that at the time that you are > using this code that you need that block to be executed? I do not > like adding more headers to this switch block. Do you think you could > fetch the importance, priority, or x-priority header(s) instead to > trigger this block's execution? > > The second patch that I don't think is needed: > > diff -urN squirrelmail/class/mime/Rfc822Header.class.php > squirrelmail-melinda/class/mime/Rfc822Header.class.php > --- squirrelmail/class/mime/Rfc822Header.class.php 2008-01-15 > 16:34:17.000000000 +0100 > +++ squirrelmail-melinda/class/mime/Rfc822Header.class.php 2008-01-15 > 16:09:56.000000000 +0100 > @@ -312,6 +312,8 @@ > case 'x-mailer': > $this->xmailer = $value; > break; > + case 'x-p772-primary-precedence': > + $this->more_headers[$field] = $value; > case 'x-priority': > case 'importance': > case 'priority': > > This is not needed for the same reason - because I think the one part > of your patch that SHOULD be added will result in $this->more_headers > having the very same information added to it anyway: > > @@ -353,6 +355,9 @@ > $this->x_spam_status = $this->parseSpamStatus($value); > break; > default: > + if (substr($field, 0, 2) == 'x-') { > + $this->more_headers[$field] = $value; > + } > break; > } > } > > Here, too, I think I see that you are trying to get the priority code > block to trigger when your header is seen too. Again, I'd rather see > this accomplished by possibly fetching the standard priority fields > (or faking them by adding them to the message object yourself). > > So again, I think these are good (but Alexandros might have something > a little different): > > diff -urN squirrelmail/class/mime/Rfc822Header.class.php > squirrelmail-melinda/class/mime/Rfc822Header.class.php > --- squirrelmail/class/mime/Rfc822Header.class.php 2008-01-15 > 16:34:17.000000000 +0100 > +++ squirrelmail-melinda/class/mime/Rfc822Header.class.php 2008-01-15 > 16:09:56.000000000 +0100 > @@ -353,6 +355,9 @@ > $this->x_spam_status = $this->parseSpamStatus($value); > break; > default: > + if (substr($field, 0, 2) == 'x-') { > + $this->more_headers[$field] = $value; > + } > break; > } > } > diff -urN squirrelmail/functions/mailbox_display.php > squirrelmail-patched/functions/mailbox_display.php > --- squirrelmail/functions/mailbox_display.php 2008-01-15 > 16:34:31.000000000 +0100 > +++ squirrelmail-patched/functions/mailbox_display.php 2008-01-24 > 10:01:37.000000000 +0100 > @@ -315,6 +315,9 @@ > } > } > > + // For X-SMTP headers we could need to fetch: > + do_hook('fetch_more_headers', &$aHeaderFields); > + > /** > * A uidset with sorted uid's is available. We can use the cache > */ Sorry, this should also have been part of the changes I personally agree with: diff -urN squirrelmail/functions/mailbox_display.php squirrelmail-patched/functions/mailbox_display.php --- squirrelmail/functions/mailbox_display.php 2008-01-15 16:34:31.000000000 +0100 +++ squirrelmail-patched/functions/mailbox_display.php 2008-01-24 10:01:37.000000000 +0100 @@ -315,6 +315,9 @@ } } + // For X-SMTP headers we could need to fetch: + do_hook('fetch_more_headers', &$aHeaderFields); + /** * A uidset with sorted uid's is available. We can use the cache */ > I'd rather see if the other parts can be addressed in a different manner. > > As for the other parts of your patch that you added in your last > message on this thread, I'd rather address them in a different thread > or different message on this thread once we resolve these. > > Thank you! > > > > I attached the latest MelindaHooks patch to this message: it was a patch > > against the 2008/01/28 version of SQM devel, so there might be rejects with the > > latest sources if you already adopted some of my patches. > > > > This new version adds the new 'next_or_previous_link' hook (which was already > > used in my previous patch for read_body.php) to the pages responsible for > > displaying the attachments, as we need the "View message" link to point back to > > the right page (in my case, either the normal read_body.php, or Melinda's one). > > The patched files are: src/image.php, src/view_text.php and src/vcard.php. As > > the mailbox cache is not readily available in these cases, I needed to change a > > bit the meaning of the first parameter passed in the 'next_or_previous_link' > > hook: it's now either an array (the cached mailbox), or it is a string (the > > mailbox name). Here is how to use that hook from a plugin: > > > > /* > > * Adjust the link for next or previous messages in the read_body menu bar, > > * and the view message link for attachments viewing pages. > > */ > > function myplugin_next_or_previous_link_do(&$argv) { > > $mailbox = $argv[0]; > > $msg = $argv[1]; > > $link = &$argv[2]; > > > > if (!is_array($mailbox)) { > > // $mailbox is only the name of the mailbox, and not the $aMailbox > > // array (this is the case when this hook is called from the > > // attachments viewing pages). So, let's retrieve that array. > > sqgetGlobalVar('mailbox_cache', $mailbox_cache, SQ_SESSION); > > if (sqgetGlobalVar('account', $temp, SQ_GET)) { > > $account = (int)$temp; > > } else { > > $account = 0; > > } > > if (isset($mailbox_cache[$account . '_' . $mailbox])) { > > $mailbox = $mailbox_cache[$account . '_' . $mailbox]; > > } else { > > // Mailbox cache not found (should never happen) ! > > // Give up instead of doing stupid things... > > return; > > } > > } > > > > if (isset($mailbox['MSG_HEADERS'][$msg])) { > > if (myplugin_is_it_my_stuff($mailbox['MSG_HEADERS'][$msg])) { > > if (strpos($link, 'read_body.php') == 0) { > > $link = '../plugins/myplugin/' . $link; > > } else { > > $link = str_replace('/src/', '/plugins/myplugin/', $link); > > } > > } else { > > $link = str_replace('/plugins/myplugin/', '/src/', $link); > > } > > } > > } > > > > where myplugin_is_it_my_stuff() is a function deciding if the message is to be > > displayed by the plugin or not, based on the array of headers it is passed. > |