Skip to content

Make Sales' emails $transport changeable from an observer #9755

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

Conversation

obuchowski
Copy link
Member

@obuchowski obuchowski commented May 24, 2017

In the commit
44791d5#diff-f625923e436a1fdf17c94533d06a45fb
$transport variable type was changed from array to \Magento\Framework\DataObject. in the
Magento/Sales/Model/Order/Email/Sender/OrderSender class

As the result OrderSender's $transport became changeable from an observer.
But OrderCommentSender, InvoiceSender and others had been missed.
This PR fixes it.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@obuchowski obuchowski force-pushed the sales_email_transport_changeable_from_observer branch from 3257ea1 to a7ac3a2 Compare May 24, 2017 20:23
@obuchowski obuchowski changed the title Sales email transport changeable from observer Make Sales' emails $transport changeable from an observer May 24, 2017
@obuchowski obuchowski force-pushed the sales_email_transport_changeable_from_observer branch 2 times, most recently from a1eb8fb to a42284b Compare May 24, 2017 21:29
@miguelbalparda miguelbalparda self-assigned this May 25, 2017
@miguelbalparda miguelbalparda added this to the May 2017 milestone May 25, 2017
@miguelbalparda
Copy link
Contributor

Hi @EObukhovsky! Thanks for your contribution, I'll check this and get back to you if we have questions!

@obuchowski
Copy link
Member Author

@miguelbalparda thanks for the quick response!

@miguelbalparda
Copy link
Contributor

@EObukhovsky this seems to be a feature instead of a bug but I still see a lot of value in the PR. It might take some extra time to process but I'll do my best have this merged. Thanks again!

@miguelbalparda
Copy link
Contributor

After reviewing this in detail, it seems the original commit you mentioned introduced a change in the code and now the event variable transport is an object instead of an array. That might be a problem for backward compatibility, source here.
This might be fixed by adding a getData() in the code to send an array or by adding an extra param, we will discuss this internally and get back to you with the next steps.
Thanks again!

@obuchowski
Copy link
Member Author

obuchowski commented May 29, 2017

@miguelbalparda good. A one more note from me:
The original problem that brought me here was a need to add an additional variable into sales emails. At the moment I forced to use workarounds like $order->setData() in observer for these purposes.
And I believe you guys find a solution.

@okorshenko okorshenko modified the milestones: May 2017, June 2017 Jun 1, 2017
@okorshenko okorshenko modified the milestones: June 2017, July 2017 Jul 2, 2017
@okorshenko okorshenko self-assigned this Jul 11, 2017
@magento-team magento-team merged commit c32d10c into magento:develop Jul 13, 2017
magento-team pushed a commit that referenced this pull request Jul 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants