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.

Sign up for GitHub

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

Jump to bottom

[2.2-develop] Transport variable can not be altered in email_invoice_set_template_vars_before Event #15040

Merged
merged 2 commits into from
May 25, 2018
Merged

[2.2-develop] Transport variable can not be altered in email_invoice_set_template_vars_before Event #15040

merged 2 commits into from
May 25, 2018

Conversation

gwharton
Copy link
Contributor

@gwharton gwharton commented May 7, 2018

Fix missing commits from #10210 Transport variable can not be altered in email_invoice_set_template_vars_before Event

Description

Original commit in #10210 missed updates to the /app/code/Magento/Sales/Model/Order/Email/Sender/OrderSender.php file

Fixed Issues (if relevant)

  1. Transport variable can not be altered in email_invoice_set_template_vars_before Event #10210

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)

gwharton added 2 commits May 7, 2018 19:03
Revert "[2.2-develop] Update Gallery Template to handle boolean confi…
ca3245f 
…g Variables"

This reverts commit 8ef039e.
Correct missing commits for #10210
874260d 
Add missing commits for \app\code\Magento\Sales\Model\Order\Email\Sender\OrderSender.php
@magento-engcom-team magento-engcom-team added this to the May 2018 milestone May 8, 2018
@VladimirZaets
Copy link
Contributor

Hi, @gwharton , I took your PR into processing, thank you for collaboration.

VladimirZaets
VladimirZaets reviewed May 8, 2018
View reviewed changes
app/code/Magento/Sales/Model/Order/Email/Sender/OrderSender.php
$this->eventManager->dispatch(
'email_order_set_template_vars_before',
['sender' => $this, 'transport' => $transport]
['sender' => $this, 'transport' => $transportObject->getData(), 'transportObject' => $transportObject]
Copy link
Contributor

@VladimirZaets VladimirZaets May 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @gwharton, due to Magento backward-compatible guide we can't rename or remove event arguments or change their type.

Adding the new arguments is allowed. If it doesn't help, you can introduce new event argument with new name or type and deprecate the old argument by adding @deprecated annotation before dispatching the event

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look at the original commit in #10210, the event arguments were changed for all email_***_set_template_vars_before events already. This PR, and the other two, are correcting the fact that the original author missed one of the 7 events that needed updating.

If this PR and the other 2 don't go through, it will leave us with 6 of the events with different types to this one.

We add a new object 'transportObject' to the event, but maintain the original 'transport' object for backwards compatability.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gwharton As I see, in 10210, that fixes were delivered to Magento 2.1 and 2.2 branches but weren't delivered to 2.3

We add a new object 'transportObject' to the event, but maintain the original 'transport' object for backwards compatability.

  • But as I see the transfer object interface was changed or I was mistaken?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the code to generate $transport has changed, but the contents and type of $transport have not changed.

@gwharton
Copy link
Contributor Author

gwharton commented May 8, 2018

$transport = $transportObject->getData() therefore the original arguments have not changed type or name.
We introduce a new argument $transportObject.
We also marked the function as deprecated.

@VladimirZaets
Copy link
Contributor

Ok, sorry, I took to progress this PR. Thanks.

@gwharton
Copy link
Contributor Author

gwharton commented May 8, 2018

Brilliant thanks. In the original PR for #10210 there are pull requests for 2.1 2.2 and 2.3, however each of the original pull requests were missing the fix for the file /app/code/Magento/Sales/Model/Order/Email/Sender/OrderSender.php

These new three PR's augment the original PR's from #10210 to make the fix complete across all three branches.

I hope this makes sense.

@gwharton gwharton mentioned this pull request May 12, 2018
Closed
@sidolov sidolov changed the base branch from 2.2-develop to 2.3-develop May 25, 2018 09:37
@sidolov sidolov changed the base branch from 2.3-develop to 2.2-develop May 25, 2018 09:38
@magento-engcom-team magento-engcom-team mentioned this pull request May 25, 2018
Closed
@magento-engcom-team magento-engcom-team merged commit 874260d into magento:2.2-develop May 25, 2018
magento-engcom-team pushed a commit that referenced this pull request May 25, 2018
ENGCOM-1467: [2.2-develop] Transport variable can not be altered in e…
56a20b7 
…mail_invoice_set_template_vars_before Event #15040
@magento-engcom-team
Copy link
Contributor

Hi @gwharton. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.2.5 release.

Please, consider porting this solution across release lines.
You may use Porting tool to port commits automatically.

@gwharton gwharton deleted the 2.2-develop-10210 branch May 25, 2018 13:37
@Karlasa Karlasa mentioned this pull request Jul 6, 2018
Closed
@gelanivishal gelanivishal mentioned this pull request Jul 17, 2018
Closed
4 tasks
magento-engcom-team added a commit that referenced this pull request Aug 1, 2018
@magento-engcom-team
ENGCOM-2555: Fix broken commit in #15040 that accidentally reverted p…
12554bb 
…revious changes. #16594

 - Merge Pull Request #16594 from gwharton/magento2:2.2-develop-16580
 - Merged commits:
   1. a012c45
magento-engcom-team pushed a commit that referenced this pull request Aug 1, 2018
ENGCOM-2555: Fix broken commit in #15040 that accidentally reverted p…
f92de64 
…revious changes. #16594
magento-engcom-team pushed a commit that referenced this pull request Aug 1, 2018
🔃 [EngCom] Public Pull Requests - 2.2-develop
e89e9bf 
Accepted Public Pull Requests:
 - #17217: [Backport] Broken Responsive Layout on Top page (by @mage2pratik)
 - #17203: [Backport] Refactored multiples conditions which could be grouped in a single on� (by @mage2pratik)
 - #17246: [Backport] FIXED: FTP user and password strings urldecoded (by @mage2pratik)
 - #17236: [Backport] Fixed invalid knockoutjs data binding for Braintree PayPal (by @tiagosampaio)
 - #16594: Fix broken commit in #15040 that accidentally reverted previous changes. (by @gwharton)
 - #17122: Added missing exception cause for better error handling (by @woutersamaey)
 - #17153: Set proper text-aligh for the <th> element of the Subtotal column in the Creditmemo email (by @TomashKhamlai)
 - #17127: Allow 3rd party modules to perform actions after totals calculation (by @navarr)
 - #16505: admin checkout agreement controllers refactor (by @AnshuMishra17)
 - #16000: Don't force enable "Use system value" checkboxes (by @likemusic)


Fixed GitHub Issues:
 - #15009: [2.2.4] Gallery theme variables being ignored (reported by @gwharton) has been fixed in #16594 by @gwharton in 2.2-develop branch
   Related commits:
     1. a012c45

 - #16580: Product gallery caption issue (reported by @Karlasa) has been fixed in #16594 by @gwharton in 2.2-develop branch
   Related commits:
     1. a012c45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VladimirZaets VladimirZaets VladimirZaets left review comments

Assignees

@VladimirZaets VladimirZaets

Labels
Component: Catalog Component: Sales Progress: accept Release Line: 2.2
Projects
None yet
Milestone
Release: 2.2.5
Development

Successfully merging this pull request may close these issues.
3 participants
@gwharton @VladimirZaets @magento-engcom-team