-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Fix "area code is not set" in all commands that depend on Payment/Helper/Data #33726
Conversation
Hi @marvinhinz. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@@ -183,7 +176,7 @@ public function getMethodFormBlock(MethodInterface $method, LayoutInterface $lay | |||
*/ | |||
public function getInfoBlock(InfoInterface $info, LayoutInterface $layout = null) | |||
{ | |||
$layout = $layout ?: $this->_layout; | |||
$layout = $layout ?: $layoutFactory->create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The $layoutFactory
variable won't exist here, you'll need to store it in a private member in the constructor to be able to use it, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I'm too slow 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized after clicking on create pr :D
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marvinhinz Thanks for your contribution.
Please look at my comment
*/ | ||
protected $_layout; | ||
protected $_layoutFactory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't rename exist properties in @api
class. It's backward incompatible.
Just add another one private property and deprecate current
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 👍🏼
@magento run all tests |
@magento run Static Tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@magento run Static Tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@magento run Static Tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@andrewbess any thoughts on why static tests are failing? |
Signed-off-by: Denis Kopylov <dkopylov@magenius.team>
Signed-off-by: Denis Kopylov <dkopylov@magenius.team>
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@ihor-sviziev I have fixed static tests and now it should be passed. I hope this PR will be proceed faster |
@magento run Functional Tests B2B, Functional Tests EE, Integration Tests, Magento Health Index, Sample Data Tests B2B, Static Tests, Unit Tests, WebAPI Tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@magento run Functional Tests B2B, Functional Tests EE, Integration Tests, Magento Health Index, Sample Data Tests B2B, Static Tests, Unit Tests, WebAPI Tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@magento run Functional Tests B2B, Functional Tests EE, Integration Tests, Magento Health Index, Sample Data Tests B2B, Static Tests, Unit Tests, WebAPI Tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
1 similar comment
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@magento run Functional Tests B2B |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
✔️ QA Passed After testing as per previous-comment, it was QA passed. But due to merge conflicts re-verified the issue again after resolving conflicts. PR is working absolutely fine. Hence forwarding the PR to Merge In Progress. |
…n Payment/Helper/Data #33726
Hi @marvinhinz, thank you for your contribution! |
Description (*)
The Helper Magento\Payment\Helper\Data creates a new layout in the constructor. This is unneccessary and creates issues when the helper is used without specifying the area code previously (eg in commands).
Edit: Also there is no real workaround to fix this. You would need to set the area in the constructor, which in turn is called every time the command list is generated, triggering "area code is already set".
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
I dont think a test case is neccessary for this change, because the only place $this->_layout is used, is in one single method as a fallback when no layout has been passed to getInfoBlock().
Maybe using the layoutfactory inside a constructor method should be prohibited at all, or at least trigger/log a warning message. It is troublesome to detect where the root issue comes from.
Contribution checklist (*)
Resolved issues: