Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
May 24, 2018

Conversation

Silarn
Copy link
Contributor

@Silarn Silarn commented May 11, 2018

…uilder classes

This fixes #65

@Silarn Silarn force-pushed the develop branch 2 times, most recently from ba7c718 to c8eea3b Compare May 11, 2018 23:59
@Silarn
Copy link
Contributor Author

Silarn commented May 12, 2018

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

@Silarn Silarn force-pushed the develop branch 6 times, most recently from 26763e3 to e9a5263 Compare May 14, 2018 17:38
@Silarn
Copy link
Contributor Author

Silarn commented May 14, 2018

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.

@Silarn Silarn force-pushed the develop branch 2 times, most recently from 3f6e840 to e1c8cbd Compare May 14, 2018 19:12
@centerax
Copy link
Member

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

@Silarn
Copy link
Contributor Author

Silarn commented May 15, 2018

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

ron72no commented May 16, 2018

When can we expect the patched version?

@centerax
Copy link
Member

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

@centerax centerax merged commit de4cab5 into ebizmarts:develop May 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants