Skip to content

Conversation

@kevinsmith
Copy link
Contributor

Haven't added tests for this yet (for the life of me, I can't get the Fetch test suite running), but in my real-world experience, these methods for decoding a message's subject and addresses have worked flawlessly.

If you can help me get the test suite running, I'm happy to write tests for these.

@kevinsmith
Copy link
Contributor Author

I also see that pull request #34 attempts to do this for subjects, but imap_utf8() has proven hit or miss for me.

@tedivm
Copy link
Member

tedivm commented Mar 14, 2014

I can see a few potential issues here. What happens if the "getSubject" function is called multiple times? It looks like the imap_mime_header_decode function gets run on the value of $this->subject, saved over that value, and then in future attempts the already decoded script will be run through imap_mime_header_decode again. At the very least this is inefficient, but at worst I can see it causing errors on subsequent requests to getSubject.

I've made some alterations to the test suite, so you should give that another try. If you have issues I'll help you out.

@kevinsmith
Copy link
Contributor Author

Merged in your latest changes and ran composer update. Seemed to get further along with booting up the VM, then this:

Dovecot has been provisioned with the test mailbox.


Environment has finished being setup
Environment Initialized


PHP Fatal error:  Call to a member function add() on a non-object in /Users/username/Sites/repos/Fetch/tests/bootstrap.php on line 43
PHP Stack trace:
PHP   1. {main}() /Users/username/Sites/repos/Fetch/vendor/phpunit/phpunit/phpunit:0
PHP   2. PHPUnit_TextUI_Command::main() /Users/username/Sites/repos/Fetch/vendor/phpunit/phpunit/phpunit:55
PHP   3. PHPUnit_TextUI_Command->run() /Users/username/Sites/repos/Fetch/vendor/phpunit/phpunit/src/TextUI/Command.php:132
PHP   4. PHPUnit_TextUI_Command->handleArguments() /Users/username/Sites/repos/Fetch/vendor/phpunit/phpunit/src/TextUI/Command.php:141
PHP   5. PHPUnit_TextUI_Command->handleBootstrap() /Users/username/Sites/repos/Fetch/vendor/phpunit/phpunit/src/TextUI/Command.php:636
PHP   6. PHPUnit_Util_Fileloader::checkAndLoad() /Users/username/Sites/repos/Fetch/vendor/phpunit/phpunit/src/TextUI/Command.php:796
PHP   7. PHPUnit_Util_Fileloader::load() /Users/username/Sites/repos/Fetch/vendor/phpunit/phpunit/src/Util/Fileloader.php:77
PHP   8. include_once() /Users/username/Sites/repos/Fetch/vendor/phpunit/phpunit/src/Util/Fileloader.php:93

Fatal error: Call to a member function add() on a non-object in /Users/username/Sites/repos/Fetch/tests/bootstrap.php on line 43

I'm running phpunit from the project's root directory. The only code changes I've made were for this pull request. Not sure what's going wrong here.

The test suite is working just fine for you? I'm wondering if there's a difference in our environments that's causing this.

@kevinsmith
Copy link
Contributor Author

I can see a few potential issues here. What happens if the "getSubject" function is called multiple times? It looks like the imap_mime_header_decode function gets run on the value of $this->subject, saved over that value, and then in future attempts the already decoded script will be run through imap_mime_header_decode again. At the very least this is inefficient, but at worst I can see it causing errors on subsequent requests to getSubject.

From my reading of the imap_mime_header_decode() docs, it looks like it won't decode an element that isn't encoded. I take that to mean that it also won't try to decode an element that has already been decoded, an assumption that my tests have confirmed. Multiple calls to getSubject return the same thing. No weird double-decoding.

Regarding efficiency, imap_mime_header_decode() is a pretty quick function. I called getSubject dozens of times on the same message, measuring the call time for each. The longest any of the calls took was only 0.00011920928955078 seconds.

@tomsommer
Copy link
Contributor

See #146

@linniksa
Copy link
Contributor

linniksa commented Aug 5, 2015

I think can be closed because #147 merged to master

@tedivm tedivm closed this Aug 5, 2015
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.

4 participants