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

Bug fixes #13

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

jkuchar
Copy link

@jkuchar jkuchar commented Dec 29, 2015

Hi! I've been migrating around 500GB of e-mails using this library. Thanks for this library you've saved me a lot of time.

However on this amount of data some unhandled edge cases poped up. This PR is summary of what was broken. There is also another PR which proposes bigger changes, which requires some more work before merging. (#14)

what has been changed:

  • introduces config files for tester (and updated readme.md)
  • Mail::getHeader() now returns NULL if header does not exist (does not make notice warnings anymore)
  • ImapDriver: properly handles long headers (that splits over more lines)
  • ImapDriver: normalizes and fixes UTF-8 encoding in mail headers
  • composer: added imap extension as dependency
  • composer: added Nette\utils as dependency because it helps a lot with fixing, normalizing, etc. UTF-8 strings

TODO:

  • There are strill some notices thrown from PHP imap extension in some cases (wasn't able to reproduce)
  • This implies another question - don't you want to use IMAP client implementation without PHP IMAP extension? (see [WIP] Experimental fixes #14 why)

@jkuchar jkuchar mentioned this pull request Dec 29, 2015
2 tasks
@jkuchar jkuchar changed the title Pack of fixes Bug fixes Dec 29, 2015
@greeny
Copy link
Collaborator

greeny commented Dec 31, 2015

Hi, thanks for your time and fixes. I am willing to merge this into master and make new release. However there is one thing, that I need to know from you. Is there any reason to add nette/utils dependency? Like you used two methods there, fixEncoding and trim. The first one can be easily copied into MailLibrary code (with proper licence info) and the second one - can it be replaced by native trim? What is the difference between these two?

The point why I dont want to create dependency is, that at the moment, you can use library without composer, just download zip and require loader.php. But with dependency, you would need to download nette/utils as well and somehow setup autoloading of that. I know, that this is maybe old-style approach, but MailLibrary 3.X was and I think should stay non-dependent on other code.

Also please note, that there is branch 4.X (not usable yet), where I am rewriting MailLibrary completely, because the code is about 4 years old and now I am a bit more experienced to not make mistakes like I did before (also there is some inconsistency and the code itself is not following Nette coding standarts, which I am using). Feel free to contact me and help me with development or just testing. It is hard to test library like this, since almost any mailserver has its own "standarts" and a little bit different behavior.

So please resolve the comments I added (or some I will add after writing this comment) and think about nette dependency (in branch 4.X, there will probably be some Nette dependency, but I think it would be good to keep 3.X standalone), I can then merge your code :)

@@ -12,9 +12,11 @@
"files": ["MailLibrary/loader.php"]
},
"require": {
"php": ">= 5.3.0"
"php": ">= 5.3.0",
"ext-imap": "*",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fix indentation to be consistent (or rather use tabs everywhere :) )

Copy link
Author

Choose a reason for hiding this comment

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

whups

@jkuchar
Copy link
Author

jkuchar commented Dec 31, 2015

However there is one thing, that I need to know from you. Is there any reason to add nette/utils dependency? Like you used two methods there, fixEncoding and trim. The first one can be easily copied into MailLibrary code (with proper licence info) and the second one - can it be replaced by native trim? What is the difference between these two?

There are also ::normalize() method used (and that is important!). Why it is so important? https://www.youtube.com/watch?v=qFfjJ8pOrWY

But with dependency, you would need to download nette/utils

You need to decide if you want to reimplement (correctly) utf8 strings manipulation or just add dependency. Because UTF-8 is really complicated to deal with I do not want to mess with it at all. There are some functions available (but requires another php extension intl)

Also please note, that there is branch 4.X (not usable yet)

Please do not "rewrite" library. Instead there is just big refactoring needed. Library as is really good. There are some architectural gotchas, but that can be fixed by few refactorings. (we can place Skype call in Czech if you wish)

@greeny
Copy link
Collaborator

greeny commented Dec 31, 2015

Also please remove imap dependency from composer. The dependency is optional since you are free to implement own IDriver. I think you can just move it to suggest part and add some comment like "Install IMAP extension, if you want to use ImapDriver".

@greeny
Copy link
Collaborator

greeny commented Dec 31, 2015

There are also ::normalize() method used (and that is important!). Why it is so important? https://vimeo.com/76597193

Yeah, but that is not part of this PR :) I think its in #14. So for this bugfix PR, I would suggest just copy fixEncoding method from Nette and use PHP trim. If thats ok for "small bugfix", which can be then released as patch version without BC break or new dependency.

Please do not "rewrite" library. Instead there is just big refactoring needed. Library as is really good. There are some architectural gotchas, but that can be fixed by few refactorings. (we can place Skype call in Czech if you wish)

Rewrite is not the correct word for this. Yes, I am writing new files, but most of the time, I am just copying old parts of code and refactoring them. Also namespaces would probably change (since MailLibrary is kinda weird, I came with MailClient, which is maybe better). Imho that is faster than refactoring old code.

I am open for skype call / text or any kind of communication :) I am also open for another PRs or even making you a contributor, if you really want to help. You can send me email or anything with your skype name (or just place it here, if you are not afraid of showing it public)

@jkuchar
Copy link
Author

jkuchar commented Dec 31, 2015

skype: honzakuchar

@greeny
Copy link
Collaborator

greeny commented Dec 31, 2015

Just sent you a message :)

@greeny greeny mentioned this pull request Dec 31, 2015
11 tasks
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