search

secit-pl / imap-bundle

Simple php-imap integration for Symfony
MIT License
71 stars 20 forks source link

Fixing Typo #37

Closed ThomasLandauer closed 10 months ago

ThomasLandauer commented 10 months ago

The important part is the wrong use line, the rest are minor rewordings.

Don't know why GitHub is showing line 299 ("The command changes its name from...") here in the diff - I didn't change anything there.

Some questions:

  1. "and using camelCased connection name": Is camelCasing really required? For me, a snake_cased foo_barConnection worked as-is.
  2. So what used to be: 
    $mailbox = $this->imap->get('foo');
$mailbox->countMails();

    ... is now:

    $this->fooConnection->getConnection()->countMails();

    Right? => The ->getConnection() step is missing here in README.

  3. The semantics are a bit irritating to me: ->getConnection() returns a Mailbox, whereas ->getMailbox() returns a string?!
secit-pl commented 10 months ago

You are right. The release was pushed out (due to lack of time) without necessary changes from previous releases (like getConnection() vs getMailbox()). I have removed the 3.0.0 release for now, and will back to it later today. I'll also take a look at this PR later today.

Thanks for the support!

secit-pl commented 10 months ago

Answering your questions:

  1. This is an internal Symfony mechanism. The idea of using such construction is based on the workflow bundle where it works very well. I haven't tested the mixed_Case notation because in my opinion it looks strange and it's not compatible with variables naming conventions from Symfony coding standards

  2. Yes, correct. Could you please link the exact place in documentation where ->getConnection() (which is now ->getMailbox()) is missing?

  3. ->getConnection() is legacy naming convention that I did not want to change to make migration to version 3.0 easier/faster. But yes, you are right, it was a bad idea to leave it as it is because it causes unintended confusion for new users. I have renamed the ->getConnection() to ->getMailbox() and ->getMailbox() to ->getImapPath() which is more intuitive for native PHPImap users.

ThomasLandauer commented 10 months ago
  1. I think it's OK now :-)