Skip to content

[2.2-Develop] Fix for issue #5726 #11308

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
Closed
wants to merge 4 commits into from
Closed

[2.2-Develop] Fix for issue #5726 #11308

wants to merge 4 commits into from

Conversation

@PieterCappelle
Copy link
Contributor

@PieterCappelle PieterCappelle commented Oct 9, 2017 •
edited by okorshenko
Loading

Was fixed in 2.0.8 branch. Never got into 2.1-develop and 2.2-develop. Cherry-pick from 9fc059b

#5726

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)

[2.2-Develop] Fix for issue #5726
a788af7 
Was fixed in 2.0.8 branch. Never got into 2.1-develop and 2.2-develop. Cherry-pick from 9fc059b
@vrann vrann added this to the October 2017 milestone Oct 9, 2017
@okorshenko
Copy link
Contributor

okorshenko commented Oct 9, 2017

Hi @PieterCappelle
Could you please check failed unit tests?

@okorshenko okorshenko self-assigned this Oct 9, 2017
@PieterCappelle PieterCappelle mentioned this pull request Oct 11, 2017
Closed
@PieterCappelle
Copy link
Contributor Author

PieterCappelle commented Oct 15, 2017

Hi, one unittest failed because of execution limit, all other ones completed without errors. I think the problem is in travis and not in my code.

@okorshenko okorshenko added 2.2.x bug report Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Oct 16, 2017
okorshenko
okorshenko reviewed Oct 25, 2017
View reviewed changes
lib/internal/Magento/Framework/Mail/Template/TransportBuilder.php
*
* @param int $scopeId
* @return $this
*/
Copy link
Contributor

@okorshenko okorshenko Oct 23, 2017

Choose a reason for hiding this comment

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

According to Backward Compatibility Policy, we cannot add new method(s) to @api classes. http://devdocs.magento.com/guides/v2.0/contributor-guide/backward-compatible-development/#introduction-of-a-method-to-a-class-or-interface

Copy link
Contributor Author

@PieterCappelle PieterCappelle Oct 26, 2017

Choose a reason for hiding this comment

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

Okay, and how should I fix that? This function is necessary for the complete workflow of this PR.

@okorshenko
Copy link
Contributor

okorshenko commented Oct 25, 2017

Hi @PieterCappelle
QA team reported GitHub issue #5726 is still reproducible on that branch. (same as on PR to 2.1-develop). Could you please double check

@PieterCappelle
Copy link
Contributor Author

PieterCappelle commented Oct 26, 2017

Can you tell me what could reproduced? Because I've edited multiple cases. Is it one specific case, because in my tests it's working like it should be.

@okorshenko
Copy link
Contributor

okorshenko commented Oct 27, 2017

Sure, I requested detailed information from the QA department. Should be ready soon

@okorshenko
Copy link
Contributor

okorshenko commented Oct 31, 2017

Hi @PieterCappelle
Here is video provided by our QA
https://youtu.be/ng4bWnkc5-Q

@PieterCappelle
Copy link
Contributor Author

PieterCappelle commented Oct 31, 2017

So strange. Viewed the video and indeed same problem occurs. Will try this case. Will keep you updated.

@okorshenko okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017
@okorshenko
Copy link
Contributor

okorshenko commented Nov 6, 2017

Hi @PieterCappelle I'm closing this PR for now. Please, reopen it once ready

@okorshenko okorshenko closed this Nov 6, 2017
@PieterCappelle PieterCappelle deleted the fix_2_2_issue_5726 branch December 21, 2017 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@okorshenko okorshenko okorshenko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@okorshenko okorshenko

Labels

bug report Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release

Projects

None yet

Milestone

November 2017

Development

Successfully merging this pull request may close these issues.

4 participants

@PieterCappelle @okorshenko @magento-team @vrann