Prevent special characters finding their way into layout handle due to SKU being used

[Marko-M]

Prevent special characters finding their way into layout handle due to SKU being used.

This avoids page cache issue when Varnish tries to retrieve ESI with invalid URL on
product page.

[Marko-M]

@maksek Resubmitted as new PR on top of 0.42.0-beta1, develop branch as instructed.

[maksek]

Thanks @Marko-M. Can you provide also some use-cases, to validate the changes.

[Marko-M]

While composing use-cases necessary to validate this pull request, I noticed that this issue has already been patched in Alpha 108, but in a different place in Magento 2 source code when compared to what's contained in this pull request. Since I prefer approach implemented by this pull request over approach used in Alpha 108, I have modified this branch to revert fix from Alpha 108 version, and will outline reasons.

Original issue was that if product has SKU containing special characters, these characters end up inside ESI URL when Varnish is used for page cache. This is because ESI src URL has layout handles as parameter, and one of the layout handles is derived directly from product SKU. Result of this issue was that catalog top nav block outputs "400 Bad Request" instead of block content due to Varnish responding to malformed URL with error.

Alpha 108+ code fixes this issue by urlencoding layout handles just before composing ESI src URL as shown here:

https://github.com/magento/magento2/blob/master/app/code/Magento/PageCache/Model/Observer/ProcessLayoutRenderElement.php#L40

In my opinion this is inferior approach because issue at hand isn't only that ESI src URL is malformed, it's that one of the programmatically added layout handles contain special characters and it shouldn't. Therefore this pull request urlencodes affected layout handles before adding them to layout, what consequently fixes page cache ESI issue. I consider this to be better approach because layout handle with special characters inside doesn't fit inside current Magento 2 environment since regular layout handles are derived from class names, and these can not have special characters inside. Additionally, if one would like to hook into such malformed layout handle, it isn't possible to create layout xml with all special characters on all file storage systems (ie. if someone uses / inside product SKU). Therefore we shouldn't allow special characters inside layout handles framework-wide, not only inside page cache module code and we should document that in such case, handle will be hook-able using its urlencoded equivalent

[verklov]

MAGETWO-32349

[kokoc]

✅ Review: passed
✅ Builds: green
✅ Resolution: ready to merge

[kokoc]

Documentation has to be updated