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

Improved error handling for message search method #445

Merged
merged 6 commits into from
Jan 28, 2020

Conversation

ikarol
Copy link

@ikarol ikarol commented Dec 30, 2019

closes #444

I wasn't able to reproduce(through tests) the issue reported in #444 as it's server specific and not trivial. However, this change doesn't break existing tests for search functionality.

@Slamdunk
Copy link
Collaborator

@ikarol
Copy link
Author

ikarol commented Jan 24, 2020

See https://github.com/ddeboer/imap/pull/436/files#r370508077

Hi,
there is one key difference in my use case and implementation vs #436
In my use case \imap_search() and \imap_sort() weren't returning false, but empty array(implying that the operation has been executed successfully), so no exception was thrown(and if you were to merge #436 , it wouldn't catch that as well for the same reason) and this error emitted notice at the end(see #444 for description) that broke my script. This was the error caused by mailbox missconfiguration.

So, my goal is to catch this kind of errors and prevent them from producing incorrect results. Now, I understand that such errors might not be relevant to InvalidSearchCriteriaException and we might need to introduce new exception type which will be responsible for errors caused "in between" search operation.

What do you think?

@Slamdunk
Copy link
Collaborator

Ok, if so please use false !== \imap_last_error() instead of imap_errors(): imap_last_error doesn't alter the error stack, so the exceptions can stay the same

@ikarol
Copy link
Author

ikarol commented Jan 24, 2020

Do you think we need a new exception type for this to separate it from search criteria specific cases?

@Slamdunk
Copy link
Collaborator

It is a good idea, but not for now it's good as is

@ikarol ikarol requested a review from Slamdunk January 24, 2020 10:14
@ikarol ikarol requested a review from Slamdunk January 24, 2020 11:08
@Slamdunk Slamdunk merged commit 208a29f into ddeboer:master Jan 28, 2020
@Slamdunk
Copy link
Collaborator

Thank you!

saintsloth pushed a commit to saintsloth/imap that referenced this pull request Oct 27, 2022
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.

Missed errors in search method
3 participants