Gracefully handle possible non-array return value of imap_getmailboxes

[Slamdunk]

Reference: #134

@robertodormepoco please can you checkout my branch imap_getmailboxes_non_array and report here the full exception error message, so I can better document what happens?

[robertodormepoco]

Done

In Connection.php line 207:

  [Ddeboer\Imap\Exception\ImapGetmailboxesException]
  imap_getmailboxes failed
  imap_alerts (0):
  imap_errors (0):

but, as I've stated before, in the C code the result is nullable, I think it would be better to check diffs between errors before and after the call, because the result value is not a valuable indicator of failure.
Though i also don't like exceptions bubbling up from private methods, the init logic for the mailbox list should be explicit and handled outside, but is the current design and that's not the point right now.

object(Ddeboer\Imap\ImapResource)#175 (2) {
  ["resource":"Ddeboer\Imap\ImapResource":private]=>
  resource(170) of type (imap)
  ["mailbox":"Ddeboer\Imap\ImapResource":private]=>
  NULL
}
string(80) "{gmail-imap.l.google.com:993/imap/notls/ssl/user="**email-address**"}"

Warning: Invalid argument supplied for foreach() in /var/app/vendor/ddeboer/imap/src/Connection.php on line 211

This was the warning for the invalid variable passed as countable to foreach, with some variable dump just ($this->resource, $this->server), and it looks fine.

[Slamdunk]

I've stated before, in the C code the result is nullable

I've read carefully your reply but honestly I have almost-zero knowledge of C and php-src

I think it would be better to check diffs between errors before and after the call, because the result value is not a valuable indicator of failure.

I don't get what you mean with this

Though i also don't like exceptions bubbling up from private methods, the init logic for the mailbox list should be explicit and handled outside

Without the current design, how would you get both lazy-loading and crappy imap extension error handling?

This was the warning for the invalid variable passed as countable to foreach, with some variable dump just ($this->resource, $this->server), and it looks fine.

What do you mean by "it looks fine"?

If you have any suggestion on how to improve this, please PR against my branch or this library.
This stuff is a pain in the ass because it's hard to test without going to the real world.

Last but not least, fa sempre piacere trovare un connazionale preparato 😉

[robertodormepoco]

I think it would be better to check diffs between errors before and after the call, because the result value is not a valuable indicator of failure.

I don't get what you mean with this

As we are not able to tell the difference between a null error and null result, we should use another source of truth for error guessing
but as i rethink there is no reason for what i wrote, it is pointless, the problem here is the imap-ext not having error semantics like using zend_throw_exception from zend_exceptions.h (such a sad story)

Though i also don't like exceptions bubbling up from private methods, the init logic for the mailbox list should be explicit and handled outside

Without the current design, how would you get both lazy-loading and crappy imap extension error handling?

Here I'm just saying that throwing exceptions from private methods hides the behaviour of the explicit/public method in the first place, the problem is not the exception/error logic

This was the warning for the invalid variable passed as countable to foreach, with some variable dump just ($this->resource, $this->server), and it looks fine.

What do you mean by "it looks fine"?

it looks fine because from the original issue someone noticed a "strange" server endpoint, but from the dump everything looks fine

I've read carefully your reply but honestly I have almost-zero knowledge of C and php-src

If you know two or three things you are good to go

Some sources about the php internals

http://www.phpinternalsbook.com/
https://externals.io/
https://github.com/tpunt/php-internals-articles
or read Extending and Embedding PHP by Sara Golemon (not updated, still the de facto book on the matter)

Ci sono tanti bravi connazionali, purtroppo non ci esponiamo troppo alla pubblica gogna. ;)

[Slamdunk]

So, AFAICT there is nothing we can improve in this imap-extension flaw, and this PR is everything we can get to handle the issue.

Nevertheless I would like at least to:

1. set up a dedicated test environment
2. reproduce the issue
3. document it

Could you describe here:

1. the email provider that generates the issue
2. the entire folder structure that allegedly is responsible for the issue

So I can reproduce it in a dedicated real email account?

[robertodormepoco]

So, AFAICT there is nothing we can improve in this imap-extension flaw, and this PR is everything we can get to handle the issue.

Nevertheless I would like at least to:

1. set up a dedicated test environment

2. reproduce the issue

3. document it

Could you describe here:

1. the email provider that generates the issue

GMail

2. the entire folder structure that allegedly is responsible for the issue

Nope NDA, sorry is my company email.

So I can reproduce it in a dedicated real email account?

I went onward testing the issue here, the problem is the missing connection due to a timeout (in my case everything greater than ~350s from the authentication to the very first IMAP message).
The ping fails, but the resource is still valid, which is misleading (and this is out of our reach, still the crappy imap-ext), so the connection liveness is not tied to the second, but only by using the first one as a check.
We should call every time \Ddeboer\Imap\Server::authenticate to revive the connection.
Therefore your PR is not the right thing to do I guess, because the exception is generated by a lack of connection to the IMAP server.

I've used this code as a proxy for a valid connection

    /**
     * @return ConnectionInterface
     *
     * @throws AuthenticationFailedException
     *
     */
    public function getConnection() : ConnectionInterface {
        if($this->connection && $this->connection->ping()) {
            return $this->connection;
        }

        $this->connection = $this->server->authenticate($this->username, $this->password);

        return $this->connection;
    }

[Slamdunk]

We should investigate imap_ping impact on performance. Take for example #326 checking everything at every step is unfeasable.