Extended features #5

latanasov · 2017-08-15T14:59:08Z

No description provided.

…patibility)

Nightbr

I think you should create a full documentation (like Symfony MailjetBundle) using mkdocs and create github pages for the full documentation.

Nightbr · 2017-08-16T08:32:00Z

README.md

    ...
-    Mailjet\LaravelMailjet\MailjetServiceProvider::class,
-    Mailjet\LaravelMailjet\MailjetMailServiceProvider::class,
+    Mailjet\LaravelMailjet\Providers\MailjetServiceProvider::class,


Add a CHANGELOG for this BC-change

Agreed, by the way I have for the moment left the Facade, for MailjetServiceProvider and implemented a separate (new) contract + service provider. Do you think I should just leave MailjetServiceProvider in the old namespace, to avoid breaking changes ?

Yep, I think we should not change this for retro-compatibility

Good, I'll leave the old facade and provider as it was and let the user choose between facade/contract approach.

arnaudbreton · 2017-08-16T09:41:40Z

src/Model/Campaign.php

+     * @param array $property
+     * @return Properties
+     */
+    public function removeOptionalProperty($property) {


If it's many properties which can be unset by a single call to this function, the parameter should better be plural, properties?

also use type array in parameters
public function removeOptionalProperty(array $property) {

In my "mind" the method was there for adding a single property,but ok.

ok so let it this way. Make sense to add or remove only one property at time

But the PHP doc above states it's an array, plus it loops on items

the loop is to delete the right item

Docs is wrong @return Properties should have been the array. Note I implement setOptionalProperties for multiple properties and addOptionalProperty for single property.

When you want to add a property you do this:

$campaign->addOptionalProperty(array("key"=>"value"));

same thing when you want to remove:

$campaign->removeOptionalProperty(array("key"=>"value"));

that's why it's and array in parameter...

yes you are right

otherwise you could do this:

public function addOptionalProperty($key, $value)

and
public function removeOptionalProperty($key)

arnaudbreton · 2017-08-16T09:43:06Z

src/Model/Contact.php

+    protected $properties;
+    protected $action;
+
+    public function __construct($email, $name = null, array $properties = [])


Why not follow same naming convention than other model, which is optionalProperties?

If name is optional, why put it in the constructor?

arnaudbreton · 2017-08-16T09:43:19Z

src/Model/Contact.php

+        }
+
+        if (!is_null($this->properties)) {
+            #$result[self::PROPERTIES_KEY] = $this->removeNullProperties($this->properties);


arnaudbreton · 2017-08-16T09:44:25Z

src/Model/EventCallbackUrl.php

+    const EVENT_TYPE_SPAM = 'spam';
+    const EVENT_TYPE_BLOCKED = 'blocked';
+    const EVENT_TYPE_UNSUB = 'unsub';
+    const EVENT_TYPE_TYPOFIX = 'typofix';


For all const, I think the best is to implement const into the wrapper and use them in all libraries. Look at this issue mailjet/mailjet-apiv3-php#82

arnaudbreton · 2017-08-16T09:44:31Z

src/Model/EventCallbackUrl.php

+    const EVENT_TYPE_UNSUB = 'unsub';
+    const EVENT_TYPE_TYPOFIX = 'typofix';
+    const EVENT_TYPE_SENT = 'sent';
+    const EVENT_TYPE_PARSEAPI = 'parseapi';


arnaudbreton · 2017-08-16T09:53:31Z