search

ebizmarts / magento2-mandrill

Mandrill integration for Magento 2.
https://mandrill.com/
36 stars 36 forks source link

Magento 2.2.4: Ensure Message objects are the same on both TransportB… #66

Closed Silarn closed 6 years ago

Silarn commented 6 years ago

…uilder classes

This fixes #65

Silarn commented 6 years ago

Working on this. Tests have to be updated and I'm trying to set up a test for the new class.

Silarn commented 6 years ago

This fix is good. The tests are updated for Magento 2.2.4. I threw in a PHP 7.1 test path for completeness but it's not required.

I'm still not certain why the default code is not using a singleton for the Mandrill Message object. It's possible there's still a DI-based fix that could correct the problem but I was unable to find it with the time I have available.

centerax commented 6 years ago

Is this still compatible with 2.1.x after this change?

Silarn commented 6 years ago

No, which is why I put the dependency into the composer file. If there's a di solution which can ensure the same message instance is used by both transport builder models without having to override SenderBuilder then you might be able to keep it backwards compatible.

ron72no commented 6 years ago

When can we expect the patched version?

centerax commented 6 years ago

Merging this will make the extension not compatible with Magento 2.1.x.

I think the solution is to have 2 branches, one for 2.1.x and one for 2.2.x given this change is not compatible with 2.1.

Then, we would have for example for 2.1 -> 3.1.x Branch (or 3.0.x) For 2.2.x -> 3.2.x