Issue #6808: Allow to specify a layout template type. #4977
Conversation
|
Related to: backdrop/backdrop-issues#6808 |
quicksketch
left a comment
There was a problem hiding this comment.
Overall looks great! I have some questions about namespaces.
Unfortunately I don't think I can write test coverage in time for the 1.32.0 feature freeze, so this likely won't make it for this release.
| * @param string $flexible_template_type | ||
| * The type of flexible template to save. Defaults to 'flexible'. | ||
| */ | ||
| function layout_flexible_tempstore_set(LayoutFlexibleTemplate $flexible_template) { | ||
| tempstore_set('layout.flexible', $flexible_template->name, $flexible_template, 604800); | ||
| function layout_flexible_tempstore_set(LayoutFlexibleTemplate $flexible_template, $flexible_template_type = 'flexible') { | ||
| tempstore_set('layout.' . $flexible_template_type, $flexible_template->name, $flexible_template, 604800); |
There was a problem hiding this comment.
Since this is only tempstore storage, we don't need to be compatible with the previous tempstore naming. I would suggest maybe making $flexible_template_type = 'default' by default, and save the tempstore location like this:
tempstore_set('layout.flexible.' . $flexible_template_type, $flexible_template->name, $flexible_template, 604800);
That makes it more clear that the tempstore entries are different kinds of flexible layout templates, and not tempstore of other layout configuration.
| $item = layout_flexible_template_load($flexible_template_name); | ||
| $caches = &backdrop_static(__FUNCTION__ . ':' . $flexible_template_type, array()); | ||
| if (!isset($caches[$flexible_template_type . ':' . $flexible_template_name])) { | ||
| if (!$item = tempstore_get('layout.' . $flexible_template_type, $flexible_template_name)) { |
There was a problem hiding this comment.
Same here, make $flexible_template_type default to default and update this to:
| if (!$item = tempstore_get('layout.' . $flexible_template_type, $flexible_template_name)) { | |
| if (!$item = tempstore_get('layout.flexible.' . $flexible_template_type, $flexible_template_name)) { |
| else { | ||
| $configs = array(); | ||
| $config_names = config_get_names_with_prefix('layout.flexible.'); | ||
| $config_names = config_get_names_with_prefix('layout.' . $flexible_type . '.'); |
There was a problem hiding this comment.
Ah, now I see. We also get into config file names here, which would be cumbersome to rename. We could possibly do something like:
| $config_names = config_get_names_with_prefix('layout.' . $flexible_type . '.'); | |
| $flexible_config_type = ($flexible_type === 'default' ? 'flexible' : 'flexible_' . $flexible_type); | |
| $config_names = config_get_names_with_prefix('layout.' . $flexible_config_type . '.'); |
Which would use the current config name for flexible templates and append the namespace to the word "flexible_" if using something else.
@laryn What kind of names are you expecting for the $flexible_type variable? If we left as-is, we'd need to safe-guard certain words like "layout" and "settings" to prevent config name clashes, which is why I would prefer doing name-spacing.
@indigoxela, if you have some suggestions on naming (having led our "layout" vs. "layout template"), I would love your feedback here.
There was a problem hiding this comment.
@quicksketch I added a change that processes the template type string and prefixes it with 'custom_' if it's not either 'flexible' or 'layout' (from core usage). See _layout_check_template_type(). I haven't changed the tempstore names or changed references to 'flexible' to be 'default' as you floated in another comment, because the template type is used in config as well, as you found here. Let me know your thoughts on the current state.
Just check if they are flexible, don't require the core flexible template to be used.
The layout ID was removed due to the new possibility of nested layouts, which should not share an ID.
|
Tugboat has finished building a preview for this pull request! Website: https://pr4977-2snumhssqoe5q0xf7592fzcm6p3tv8lw.tugboatqa.com/ This preview will automatically expire on the 19th of February, 2026. |
Fixes backdrop/backdrop-issues#6808