Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Experimental fixes #14

Closed
wants to merge 11 commits into from

Conversation

jkuchar
Copy link

@jkuchar jkuchar commented Dec 29, 2015

This PR contains experimental fixes and idea proposals. Comments please.

  • Mail API: fix of header name formating (access using magic methods) (BC break)
  • ImapDriver: removed server prefix from folder listing (BC break) related to Imap move/copy does not work when mailbox prefixed with server name #10
    • TODO: This should be configurable instead.
  • ImapDriver does not parse address fields into ContactList object due to very buggy implementation of buggy addr list parser implementation (BC break)
    • TODO: write paser for that or provide option if you want to parse it or not
    • BTW I do not think that it is good idea to have parsing of this level in IMAP driver. This should be on higher level - e.g. in Mail or not at all and provide extra service for that.
  • Another thing that should be mentioned is that php_imap extension is very buggy and it behaves stupid in many cases. E.g. mail->move() will copy and delete original message. (read from mail server logs) This and other bugs can be solver by implementing standalone IMAP client that will be not dependent on php_imap extension. Only working library for this case looks to be http://dev.horde.org/imap_client/ which is 100% PHP implementation of IMAP (no extensions required)

This PR requires #13 to be merged first.

@jkuchar jkuchar mentioned this pull request Dec 29, 2015
2 tasks
@@ -381,7 +385,7 @@ public function setFlag($mailId, $flag, $value)
* @throws DriverException
*/
public function copyMail($mailId, $toMailbox) {
if(!imap_mail_copy($this->resource, $mailId, $this->server . $this->encodeMailboxName($toMailbox), CP_UID)) {
if(!imap_mail_copy($this->resource, $mailId, /*$this->server .*/ $this->encodeMailboxName($toMailbox), CP_UID)) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be some way how to configure this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment in PR

@jkuchar jkuchar changed the title Experimental fixes [WIP] Experimental fixes Dec 29, 2015
…hitespaces, normalizes UTF-8 strings and trims headers content
…roper bullet proof adrlist parser; PHP one is not realible at all
…was totaly unpredictable and caused a lot of pain

PROPOSAL: getHeader should return what was found just by case insensitive comparison; object properties can be magically mapped; maybe better to use ArrayAccess where you can use proper header names?
@greeny
Copy link
Collaborator

greeny commented Dec 31, 2015

Mail API: fix of header name formating (access using magic methods) (BC break)

I was thinking about this, and I think the best solution is:

  • parse headers but DONT change keys, save in array (duplicite headers if any - only last one is used)
  • access using ->getHeader("X-Requested-With") - access directly target header (no magic here)
  • access using ->xRequestedWith - change camelCase into kebab-case with uppercased first letters in each word

I think this should work without any problems, do you have other opinion?

ImapDriver: removed server prefix from folder listing (BC break) related to #10

This is problem I had not figured out yet, how to solve. I am planning to do little test for most common email providers and check, how does their IMAP server react to create/rename/delete mailbox and move/copy/delete mail from mailbox, with both prefixed and non-prefixed mailbox name. I know, that 4 years ago when I was writing this library, I did some testing for gmail and this worked, so I used it and let it be. But things maybe changed or you used different provider, so this needs to be tested.

Since it is BC break, this should be only in 3.3.0 or even 4.0 (which is already in development, slow, but it is)

ImapDriver does not parse address fields into ContactList object due to very buggy implementation of buggy addr list parser implementation (BC break)
TODO: write paser for that or provide option if you want to parse it or not
BTW I do not think that it is good idea to have parsing of this level in IMAP driver. This should be on higher level - e.g. in Mail or not at all and provide extra service for that.

I completely agree this is not the best implementation. I wanted to provide easier access to parts of address, but did not think about it correctly (also somebody came with PR, which I merged without thinking, thats why there is some buggy code).

I think getHeader() should return raw header data, not parsed or anything. And I have to decide, if there will be some other method (e.g. getContactHeader()), which will provide parsed data, or if I will split this into some service.

The reason why it is in IMAP driver and not on higher level is because it uses some imap_* function for parsing. I am willing to write custom parser based on same RFC and split the functionality into some other service.

Another thing that should be mentioned is that php_imap extension is very buggy and it behaves stupid in many cases. E.g. mail->move() will copy and delete original message. (read from mail server logs) This and other bugs can be solver by implementing standalone IMAP client that will be not dependent on php_imap extension. Only working library for this case looks to be http://dev.horde.org/imap_client/ which is 100% PHP implementation of IMAP (no extensions required)

That is part of refactoring in 4.X branch, which I will think about. Thanks for the link, I was looking some time ago for working library and did not find any. Maybe I can use that, or just write own client. It will be recommended driver, but I think I should not drop support for IMAP driver. Or should I?

@jkuchar
Copy link
Author

jkuchar commented Dec 31, 2015

header: I think this should work without any problems, do you have other opinion?

No this will not work. What i come with works. parse header -> fix utf8 -> utf8 NFC normalization -> toLower -> save as key to array This is only what should works. E.g.: Cc, CC and cc is the same header

server prefix

This should be configurable. And default to be turned on. So no BC break. I've been using it with Kerio Connect.

Or there can be also auto-detection using mail listing. But this can be slow in big mailboxes.

The reason why it is in IMAP driver and not on higher level is because it uses some imap_* function for parsing. I am willing to write custom parser based on same RFC and split the functionality into some other service.

Cool!

imap driver without imap extension
I thing there should be MailClient library in one repo and drivers in another (because of dependencies). You you wish to continue developing driver for PHP imap extension you can but I do not see any advatages over using horde client.

@greeny greeny mentioned this pull request Dec 31, 2015
11 tasks
@jkuchar jkuchar closed this Oct 5, 2017
@jkuchar jkuchar deleted the experimental-fixes branch October 5, 2017 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants