Added option for setting fetched emails \\seen flag to seen #7
Conversation
mnapoli
left a comment
mnapoli
left a comment
There was a problem hiding this comment.
Thank you for the contribution. I am not sure I understand everything here.
Will it change the behavior of the library? I.e. you set the default value for $peek as true -> is that already the case today?
And given the code, when you set it tofalse it doesn't seem to do anything different when the query is sent.
| * @param boolean $peek | ||
| */ | ||
| public function setPeek($peek) { | ||
| $this->peek = $peek; |
There was a problem hiding this comment.
The $this->peek variable is set but it seems it's never read?
There was a problem hiding this comment.
Yeah sure for some reasons the read method was not in the version i pushed.
| * @return Email[] | ||
| */ | ||
| public function getEmails(Query $query = null) : array | ||
| { |
There was a problem hiding this comment.
The { should be on a new line (this is the PSR-2 coding standard). This comment applies also below on other methods.
There was a problem hiding this comment.
Ok IDE auto format will sort it out.
| if ($query->getYoungerThan() !== null) { | ||
| $hordeQuery->intervalSearch( | ||
| $query->getYoungerThan(), | ||
| Horde_Imap_Client_Search_Query::INTERVAL_YOUNGER |
There was a problem hiding this comment.
Why are those lines changed? Those were formatted to follow PSR-2.
There was a problem hiding this comment.
Could you please revert those changes?
src/Client.php
Outdated
| } | ||
|
|
||
| /** | ||
| * @param String $id Description |
There was a problem hiding this comment.
This line should be removed (there is no additional information, the type-hint is enough)
| /** | ||
| * @param String $id Description | ||
| * @param boolean $peek sets the peek option, "false" fetched emails' state will be set to seen "true" no change of state default is true | ||
| * @param String $folder the folder to get email from |
|
I dont know how but the get method for property peek was not in the version i pushed, As if that is not funny enough, the line 196 which is the whole point of the contribution was also not changed. fixed it. |
|
Did fix the issues, did you get some time to check it? |
| * | ||
| * @var boolean | ||
| */ | ||
| private $peek; |
There was a problem hiding this comment.
You should not store state into a property in this class. The Client class is a service, it should be stateless.
You should instead pass the $peek value in parameters of method calls.
|
|
||
| ->getQuery(); | ||
|
|
||
| $emails = $client->getEmails($query,'INBOX', false); // set to "false" fetched emails' state will be set to seen. "true" no change of state will occur. default is true |
There was a problem hiding this comment.
Missing a space between $query,'INBOX'
Also could you put the comment at the start of the line above? That will be easier to read, else there will be a scrollbar on GitHub.
| const FLAG_DRAFT = 'DRAFT'; | ||
| const FLAG_FLAGED = 'FLAGGED'; | ||
| const FLAG_RECENT = 'RECENT'; | ||
| const FLAG_SEEN = 'SEEN'; |
2 participants
Hi
I just added option for setting fetched emails \seen flag to seen. i added an option on both the
getEmails()andgetEmailIds()methods as a second optional argument. Advise if its ok. thanks