-
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
magento/magento2#24730: Fix Frontend Invoice PDF configured Logo image #30632
magento/magento2#24730: Fix Frontend Invoice PDF configured Logo image #30632
Conversation
Hi @mozok. 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 give me test instance |
Hi @mozok. Thank you for your request. I'm working on Magento instance for you. |
Hi @mozok, here is your Magento Instance: https://36f7c4dd56c3255766661f9a76fa8005.instances.magento-community.engineering |
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.
Hi @mozok
Thanks for your contribution
Please take a look at my comments and consider covering your functionality by tests. (I think integration test is the best fit here)
<referenceContainer name="header-wrapper"> | ||
<referenceBlock name="logo"> | ||
<arguments> | ||
<argument name="logo_src" xsi:type="helper" helper="Magento\Sales\Helper\Invoice\Logo::getLogoFile"/> |
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 change xsi:type, I believe it should be "object" instead of "helper"
Also for block it's better to use ViewModel
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.
Here I rely on check in original Block class
\Magento\Theme\Block\Html\Header\Logo
public function getLogoSrc()
{
if (empty($this->_data['logo_src'])) {
$this->_data['logo_src'] = $this->_getLogoUrl();
}
return $this->_data['logo_src'];
}
So I want to inject result of my service class execution into 'logo_src' argument just in one layout.
I assume, that give me opportunity to change logo for one layout, and not modify original template/block class.
Move Custom Logo file getter to separate service class
simplify execute method
@magento run all tests |
@magento run Integration Tests |
@magento run Functional Tests EE |
1 similar comment
@magento run Functional Tests EE |
@magento run Functional Tests EE This test is failing on different 'flaky' tests ( |
✔️ QA Passed The result is the same as in the comment above |
@magento run all tests |
again error in "flaky" test) |
Hi @mozok, thank you for your contribution! |
Description (*)
According to #24730 Admin Configuration Stores -> Configuration -> Sales -> Sales ->'Invoice and Packaging Slip Design' -> 'Logo for HTML Print View' does not affect on logo on frontend invoice print page.
sales_order_printinvoice page use general theme class/template for Logo generation - app/code/Magento/Theme/Block/Html/Header/Logo.php
In Block 'logo_src' argument is checked, before logo path is generated, so I use it as extension point to insert logo configuration on print invoice page.
Changes affects only frontend sales order print invoice page through layout file. Similar approach can be used to change other print pages, like sales_order_print.
Related Pull Requests
no
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
no
Contribution checklist (*)