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.

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

Add dispatching of view_block_abstract_to_html_after event #3779

Merged
merged 2 commits into from
Sep 28, 2016
Merged

Add dispatching of view_block_abstract_to_html_after event #3779

merged 2 commits into from
Sep 28, 2016

Conversation

aleron75
Copy link
Contributor

Just like it was in Magento 1 Mage_Core_Block_Abstract class, this change is aimed at having an event dispatched after the HTML is generated by the toHtml() method.

This can be used to change HTML without the need to implement a plugin for the Magento\Framework\View\Element\AbstractBlock class.

@@ -665,6 +665,15 @@ public function toHtml()
}
$html = $this->_afterToHtml($html);

/** @var \Magento\Framework\DataObject */
$transportObject = new \Magento\Framework\DataObject(
Copy link
Contributor

Choose a reason for hiding this comment

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

There are at least 2 reasons not to trigger this event in AbstractBlock:

  1. Abstract block is overloaded with behavior
  2. If some block will just implement BlockInterface (not extend from AbstractBlock), this event will not be called.

Overall approach of Magento 2 is to use composition instead of inheritance to reuse behavior. In this case you need to reuse behavior common for all toHtml() calls. Please consider putting this behavior to \Magento\Framework\View\Layout::renderNonCachedElement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comment.

I followed the example of view_block_abstract_to_html_before event which is dispatched at the beginning of the toHtml() method: in what does the approach of dispatching this event differ from mine?

Regarding your suggestion of putting the event in the \Magento\Framework\View\Layout::renderNonCachedElement do you suggest to add both a before and an after event?

Furthermore, would you suggest to differentiate the event based on the element (UI component, Block or Container) or should I keep the same event for all three?

Cheers,
Alessandro

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked closer at implementation. Please disregard my comment.

While it would be more correct to put triggering of both before- and after- events in layout, Magenot is not ready for that, since in many places Block::toHtml() is triggered directly.

@piotrekkaminski
Copy link
Contributor

internal issue MAGETWO-54354

@hshar7
Copy link
Contributor

hshar7 commented Jul 28, 2016

@aleron75 could you please fix unit tests. Many of them are failing after I merged latest mainline.

@vasiliyseleznev vasiliyseleznev removed the MX label Aug 3, 2016
@vkorotun vkorotun added Area: Frontend improvement Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development and removed Progress: needs update Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development labels Aug 3, 2016
@vkorotun
Copy link
Contributor

vkorotun commented Aug 8, 2016

Internal ticket: MAGETWO-56556

@vkorotun vkorotun added the linked label Aug 8, 2016
@vkorotun vkorotun added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development and removed linked labels Aug 22, 2016
@vkorotun vkorotun self-assigned this Sep 14, 2016
@mmansoor-magento mmansoor-magento merged commit 05b40d1 into magento:develop Sep 28, 2016
@Ctucker9233
Copy link

@mmansoorebay Will this PR be included in 2.1.3?

magento-engcom-team pushed a commit that referenced this pull request Feb 22, 2019
[pangolins] Convert MTF tests to MFTF tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Frontend improvement Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Progress: accept
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants