cancel
Showing results for 
Search instead for 
Did you mean: 

Containers and Blocks - Possible Changes

akent99
Regular Contributor

While chatting with some folks in the MageFront Slack channel, a question came up of “should HTML be removed from layout files.” Currently containers in the layout files have HTML markup in attributes. It avoids “yet another template file” but at the cost of mixing HTML into layout files blurring the line between markup and structural layout information.

 

Containers vs Blocks

One of the changes in Magento 2 was the introduction of containers as distinct from blocks. The goal was:

 

  • Blocks have a fixed set of named children
  • Containers have an array of anonymous children

 

One question that arose was whether to have template files for containers. As most containers were a single HTML element (typically a “div” element) it was deemed adequate to put the HTML markup directly into the container nodes. It improves performance (estimated at up to 5% of a page load) and reduces the overall file count. The negative however is it mixes HTML (presentation) into the more structural layout files. There was a discussion on the MageFront slack channel around separating HTML out of layout files to have a cleaner separation, leading to this blog post.

 

So, what could be done to introduce such a change?

 

Option 1 – Do Nothing

We could just do nothing. It’s not that bad. Other more important fish to fry. (Can anyone say “PWA”?)

 

Option 2 – Add Templates to Containers

One change that could be made is to add a “template” attribute to existing container nodes. If specified instead of the HTML attributes, the referenced PHTML template file is used. That would keep the concept of templates, but grant the full PHTML capabilities of other blocks. Such template files may be short in general, but you get complete flexibility.

 

This approach has the benefit of minimal impact while allowing HTML to be extracted from layout files. The only issue to be careful of is that themes and extensions can change the HTML attributes of container nodes already – so such changes would somehow need to override the use of a PHTML template file (last change wins).

 

Option 3 – Phase Out Containers

A more radical change is to phase out the usage of containers and turn them all into blocks. Blocks today still can have an array of children, so leverage that capability. All the current layout files would be changed to remove <container> nodes and turn them into <block> nodes with a simple PHTML template.

 

This would have a bigger impact – all <referenceContainer> elements in layout files used by extensions and themes would also have to be changed to <referenceBlock> elements.

 

The benefit of this approach is it removes a concept from Magento (containers). Everything is just blocks (and UI components). It would however take more effort to roll out.

 

Personally, I think option 2 is best. It allows separation of HTML from layout files which is semantically cleaner with the least effort. I don’t think option 3 is worth the effort.

 

Context and the Render Hierarchy

The above does however feed into a related question of what to do about standardization of children of blocks. Should we continue with the drive to always have a single child per alias name and remove the ability to have an array of children in blocks?

 

Currently, inside a PHTML file you can have a getChild(‘alias-name’) call to fetch a child block by name. There should be exactly one child with that name. The parent code can then pass down context by calling ‘set’ methods on the child block before asking the child to render itself.

 

There can be a problem when you want to add a block alongside the existing named child block. Currently, a new container must be inserted above the child block (which is possible but awkward). In some cases, this will stop contextual information being passed down to the child block (the ‘set’ methods will be invoked on the container rather than the block, unless containers are modified to propagate all ‘set’ calls down to all children).

 

A different strategy here would be to eliminate the practice of parent blocks calling ‘set’ methods on child blocks and instead pass context information down to children instead (like “props” in ReactJS). For example,

getChild(‘alias-name’, [ ‘product_id’ => $prodId ])

would pass all the inherited properties plus a new “product_id” property down the tree. That is, as the rendering tree is walked, parent nodes can define additional properties that are set and made available to all descendants in the tree (not just the immediate children). A parent node may, for example, fetch a product description entity from the database allowing different descendant blocks to render information from the product description. The child blocks are documented as to what information they assume will be available in context – parent blocks are responsible to provide that information.

 

Named Block Children

If context is introduced as above, another form of simplification could be introduced. With the above change to the rendering call graph in place, all ‘set’ calls for passing context information down to children can be eliminated. Named children then no longer need to be single blocks. Instead, every alias name could allow a list of blocks instead of a single block. Adding new children would no longer need new containers to be inserted into the tree just to allow a second sibling to an existing block. Everywhere a block is allowed today, zero or more blocks could exist instead.

 

In this proposal, containers can be left in place for backwards compatibility – the enhancement would be that adding an extra child under an existing block could be done without introducing a new container above it.

 

Conclusions

This blog post explored a few possible changes. Feedback is welcome on three separate topics:

 

  1. Is it worth changing how context is passed to child blocks?
  2. Is it worth making a change to allow HTML for containers to be in PHTML template files instead of embedded in layout files? If so, which option do you prefer?
  3. Is it worth allowing an array of children everywhere getChild(‘alias-name’) is used today so fewer containers would need to be used in layout files?

The third change would be a bigger impact to the code base.

5 Comments
dewimorgan
Senior Member

Make no change unless it is the Right Change. Any other approach is littered with the jagged edges of cruft and code debt.

 

Any time I find myself saying "I could make a change... I don't have to... but doing it properly is too much effort" then I should either make no change, or bite the bullet and make the hard change. Any other path, I will regret.

 

Option 3 is doing it properly, simplifying the templating system down to lower-learning-curve elegance. This elegance comes at the cost of pain - but excisions always do.

 

It feels shortsighted to handwave that benefit away as "not worth" the temporary pain of the excision, but then suggest wasting time on the bandaid-on-a-skunk solution 2, slathering on an extra layer of permanently painful complexity instead of elegantifying.

 

Better then, to pick option 1, taking the Hippocratic path, and cause no pain at all.

akent99
Regular Contributor

Thanks for the comment. I am a bit in the "leave it for now until the PWA story is clearer", but wanted to judge how big a problem for the community as a whole. 

BenCrook
M1 Certified

I'm not a huge fan of having markup/classes inside XML files but I wouldn't say it's a large issue that needs re-working, at this point I completely agree that there are bigger fish to fry and this should be reviewed in a year or so.

 

I'm leaving the office shortly so I don't have enough time to go into any more detail sorry.

Bartek Igielski
M1 Certified

If this can be delivered to stable release relatively fast (2.3), I'd opt for option number 2., because any way of rendering HTML through non-template code is painful to work with and should be exterminated as soon as possible.

 

I'm currently rewriting header to not use any container at all, b/c we are wasting lots of time working with this part for every client (it's like 50%+ more time compared to M1).

swagner
M2 Certified

I agree with @dewimorgan and would opt for option 3 as it is IMHO a non-optimal design decision in the very first place and maybe should never have been implemented the way it is. So I'd rather take the bitter pill for the sake of a clean and solid base.

 

As for backwards compatibility - I don't know if it would be possible to introduce such a heavy and most possibly breaking change as an isolated module?

 

Option 2 would for sure help in some places, but it leaves many issues open. You named one explicitly. I think general debugging will not get any better, too.

 

Lastly I don't know the state of that PWA thingy. But by know this sounds too fuzzy and vague to me to be a valid factor for this consideration.