Added option for setting fetched emails \\seen flag to seen by katwekibs · Pull Request #7 · mnapoli/imapi

katwekibs · 2017-11-11T16:50:21Z

Hi
I just added option for setting fetched emails \seen flag to seen. i added an option on both the getEmails() and getEmailIds() methods as a second optional argument. Advise if its ok. thanks

mnapoli

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.

mnapoli · 2017-11-11T17:00:20Z

src/Client.php

+     * @param boolean $peek
+     */
+    public function setPeek($peek) {
+        $this->peek = $peek;


The $this->peek variable is set but it seems it's never read?

Yeah sure for some reasons the read method was not in the version i pushed.

mnapoli · 2017-11-11T17:01:14Z

src/Client.php

     * @return Email[]
     */
-    public function getEmails(Query $query = null) : array
-    {


The { should be on a new line (this is the PSR-2 coding standard). This comment applies also below on other methods.

Ok IDE auto format will sort it out.

mnapoli · 2017-11-11T17:02:02Z

src/Client.php

        if ($query->getYoungerThan() !== null) {
            $hordeQuery->intervalSearch(
-                $query->getYoungerThan(),
-                Horde_Imap_Client_Search_Query::INTERVAL_YOUNGER


Why are those lines changed? Those were formatted to follow PSR-2.

Could you please revert those changes?

mnapoli · 2017-11-11T17:02:43Z

src/Client.php

    }

    /**
+     * @param String $id Description


This line should be removed (there is no additional information, the type-hint is enough)

mnapoli · 2017-11-11T17:03:00Z

src/Client.php

    /**
+     * @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


String -> string

katwekibs · 2017-11-11T22:02:24Z

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.

$query->fullText([
           'peek' => $this->getPeek(),
       ]);

fixed it.

katwekibs · 2017-11-16T12:06:26Z

Did fix the issues, did you get some time to check it?

mnapoli · 2017-11-18T11:12:49Z

src/Client.php

+     * 
+     * @var boolean
+     */
+    private $peek;


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.

mnapoli · 2017-11-18T11:14:04Z

README.md

+
+    ->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


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.

mnapoli · 2017-11-18T11:15:10Z

src/Query/Query.php

+    const FLAG_DRAFT = 'DRAFT';
+    const FLAG_FLAGED = 'FLAGGED';
+    const FLAG_RECENT = 'RECENT';
+    const FLAG_SEEN = 'SEEN';


Thanks for fixing this!

added option for setting fetched emails \\seen flag to seen

4d130ef

mnapoli reviewed Nov 11, 2017

View reviewed changes

katwekibs added 2 commits November 12, 2017 00:31

Fixed corrections as suggested by @mnapoli.

14bf8a0

Fixed code to follow PSR-2 code formating standards

b8c98f1

mnapoli reviewed Nov 18, 2017

View reviewed changes


		->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

Conversation

katwekibs commented Nov 11, 2017

Uh oh!

mnapoli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnapoli Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

katwekibs commented Nov 11, 2017

Uh oh!

katwekibs commented Nov 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mnapoli Nov 18, 2017 •

edited

Loading