Move custom layout updates #187
Conversation
| is via user forms more commonly used by content managers. That's the __1st problem__ - content managers don't | ||
| need that functionality and it is better to hide it from them to avoid them from executing PHP code on the server. | ||
|
|
||
| __2nd problem__ is that we do not allow developers to work with entity-specific layout updates in a developer way. |
There was a problem hiding this comment.
I think, this is possible currently, we add dynamic layout handle that contain entity identifiers. But it's hard to manage because in the code you depend on dynamic identifiers.
There was a problem hiding this comment.
But then it's impossible for content managers to control these updates, with the proposed approach content managers will still be able to create staging updates using layout updates prepared by developers.
There was a problem hiding this comment.
The statement is incorrect, it doesn't say anything about content managers.
| Meaning that there is no way to use version control while editing these layout updates. | ||
|
|
||
| ### Solution | ||
| Remove custom layout updates from admin panel, ignore custom_layout_update properties updated via web API and import |
There was a problem hiding this comment.
This will not work with content staging.
There was a problem hiding this comment.
I've updated the document so this could work with different stores and staging updates
|
@melnikovi from my POV, this is something we should do in this way or another. We see many possible vulnerabilities in layout updates XML and whitelisting only allowed classes could easily miss a valid use case. Blacklisting doesn't work due to the number of possible classes that can be abused. Moving it to code level would make it much harder to exploit as would require file-level access. |
|
According to BC policy, at first, we need to mark the functionality as deprecated and support it during 1 minor release. I would propose, to remove a possibility to add layout updates via Admin UI configuration and mark all related code as deprecated. |
… move-specific-layout-edit
I've added a bit regarding backward compatibility |
| Read custom layout updates from physical files on the server. Convert existing textarea _custom_layout_update_ field | ||
| in category/product/cms page form into a _select_ field where existing custom layout update files could be selected. | ||
| A developer would create a layout file following this template: | ||
| _app/design/custom\_layout/\<category/product/page\>/layout\_update\_\<entity ID\>\_\<readable layout update name\>.xml_. |
There was a problem hiding this comment.
I would propose to put layouts in media folder. What do you think @buskamuza?
There was a problem hiding this comment.
Putting them in media would make it the same as it is right now. As I understand, the intent is to give an ability for the admin user to upload the files.
| A developer would create a layout file following this template: | ||
| _app/design/custom\_layout/\<category/product/page\>/layout\_update\_\<entity ID\>\_\<readable layout update name\>.xml_. | ||
|
|
||
| e.g. if a file with the name _app/design/custom\_layout/page/layout\_update\_2\_store1update.xml_ is created then |
There was a problem hiding this comment.
I think there is already a way to create handles that can contain id of the entity and they will be applied automatically. Maybe it would be better not to have ids in the layout names here, to not make it confusing?
|
Approved with the following suggestions:
|
| ### The problem | ||
| Layout updates are defined using XML and certain instructions often using PHP classes. Only developers can | ||
| posses knowledge required to create a layout update yet the only way to provide these updates | ||
| is via user forms more commonly used by content managers. That's the __1st problem__ - content managers don't |
There was a problem hiding this comment.
Are you sure that no one from content managers are using this functionality? Removing it will be backward incompatible change. Not sure that’s good idea at all.
Also removing this functionality will lead to inability to apply some layout update without development teams —> will be more expensive from merchant perspective and will not allow to do quick fixes where it’s needed
There was a problem hiding this comment.
a merchant can just put those files to certain folders themselves, it is slightly harder but definitely more secure
There was a problem hiding this comment.
@AlexMaxHorkun Many merchants don't have write access to the filesystem themselves if working with an agency.
|
the functionality should be remove but different approach will be taken |
|
disregard previos comment, value of this proposal is to allow to remove layout updates sooner in a patch version |
|
looks good to me |
| Read custom layout updates from physical files on the server. Convert existing textarea _custom_layout_update_ field | ||
| in category/product/cms page form into a _select_ field where existing custom layout update files could be selected. | ||
| A developer would create a layout file following this template: | ||
| _app/design/custom\_layout/\<category/product/page\>/layout\_update\_\<entity ID\>\_\<readable layout update name\>.xml_. |
There was a problem hiding this comment.
It's confusing that in case of a single layout update, a developer can use old approach with catalog_product_view_id_128.xml, while as soon as it's necessary to support multiple templates, a developer should put the layout updates in an absolutely different folder (app/design/custom_layout/page/layout_update_128_store1update.xml). Btw, the rule for forming the name is not very clear. There is no correlation with default layout update (cms_page_view.xml). IMO, it will be hard to find all applied layouts intuitively.
Also, it introduces new concept - custom_layout is not a theme, though it looks like a theme based on the location. We already have base, which is confusing in the same way.
I propose to consider an option of reusing existing functionality by extending it so it supports multiple layouts that can be selected by an admin.
For example, in addition to catalog_product_view_id_128.xml, also support:
catalog_product_view_id_128/layout1.xml,catalog_product_view_id_128/layout2.xml- or
catalog_product_view_id_128_layout1.xml,catalog_product_view_id_128_layout2.xml
Location will be the same as supported now: a custom module(s) or a custom theme(s), whatever works best for specific case.
There was a problem hiding this comment.
It was brought to my attention that this functionality may be used by merchants without a dedicated development team and was trying to make it easier for them to create these layout updates files. It may be a bit harder for such merchants to register a new module/theme. But I don't mind using existing target directories for these updates, it would be better not to introduce too many new conventions. @piotrekkaminski What do you think?
Same goes to naming, since these custom updates are meant to replace the updates from the admin panel which only allows to alter layouts for 'view' pages I didn't think it was necessary to follow these conventions which allow to also redefine admin 'Edit' pages. But again I understand the value of preserving existing conventions as much as possible
There was a problem hiding this comment.
@buskamuza Further I think it would be better to have old way single-entity updates and these new updates separately - when a developer create a catalog_product_view_id_128.xml style file they won't expect it to be controlled by content managers via admin panel
|
This proposal seems to be taking an unnecessarily complicated approach to solving a simple problem. It's a reasonable point that both for stability and security someone with simple content manager permissions should not have access to modify layout XML. However, this is easily solved by introducing a new ACL permission controlling access to editing admin layout XML that can be controlled separately from general content management. That way it can be restricted from a non-developer content manager while still allowing access by those who need it. Edited to remove additional objections that completely missed the point of the PR due to my poor reading the first time around. 🙄 |
that's actually already done in 2.3.2 |
Problem
With custom layout updates available only via admin panel content managers have access to functionality meant for developers, developers cannot use VCS for layout updates.
Solution
Remove custom layout updates from admin panel, allow custom layout updates by putting .xml files in a certain folder
Requested Reviewers
@melnikovi @buskamuza