Check for empty regions directly in twig file for better performance

[laurent-b4y]

Yves brought my attention on a possible performance hit Kiso has by checking the regions content in a preprocess iso directly in twig.

We should remove the preprocess used to check the "has_navigation" and "has_complementary" and replace them with something like this in the page.html.twig file

{% set page_navigaton = page.navigation|render %}
{% set page_complementary = page.complementary|render %}

{% if page_navigation|striptags|trim is not empty %}
	{% set has_navigation = true %}
{% else %}
	{% set has_navigation = false %}
{% endif %}

{% if page_complementary|striptags|trim is not empty %}
	{% set has_complementary = true %}
{% else %}
	{% set has_complementary = false %}
{% endif %}

The rest of the file doesn't need changes AFAIK.

[GyD]

To give more detail <?php render function do not return the final output.
That could lead to false result, example: when a drupal placeholder return nothing.

This can be tested with facets module and facet option that do not render the facets when the facet source is not rendered.

Laurent solution is the one we put on werkenvoor but may not be the best since this issue need investigation.

Example, with facet we currently have this output in php on page where facet are not rendered:

'  <drupal-render-placeholder callback="Drupal\\block\\BlockViewBuilder::lazyBuilder" arguments="0=category&amp;1=full&amp;2" token="3H-ATSLp_IKUKYnhOjRX3yjcYQjgLT6WOepB79hscJI"></drupal-render-placeholder>
'

But when you render in twig it return

''

Please note that solution implemented in Kiso also render region and their content multiples times (at least twice), that could lead to performances issues.

[smillart]

Please investigate and provide me with the best solution or better than what we have now. In that case, I will add this issue in the Kiso (基礎) 2.7.3 milestone and I will implement this change request.

[rutiolma]

Kiso should skip drupal generated HTML tags, like drupal-render-placeholder or drupal-entity (ideally, a rule to skip tags starting with "drupal-" would be the ideal). This would avoid false results as mentioned by GyD

[smillart]

@GyD @rutiolma I'm not quite sure exactly what I'm supposed to do. Should I delete the following PHP code and only use twig templates to test for empty regions?

kiso/includes/preprocess.page.inc

Lines 12 to 17 in 4035ba0

	// Add boolean variables detecting if regions are empty.
	$theme = \Drupal::theme()->getActiveTheme()->getName();
	$regions = system_region_list($theme);
	foreach ($regions as $key => $value) {
	$variables['has_' . $key] = _kiso_has_region(\Drupal::service('renderer')->render($variables['page'][$key]));
	}

kiso/includes/preprocess.utils.inc

Lines 22 to 34 in 4035ba0

	function _kiso_has_region(?string $markup, string $allowed_tags = '') {
	$non_conditional_html_comments_pattern = '/<!--(.|\s)*?-->\s*|\r|\n/';
	$cleaned_region_output = preg_replace($non_conditional_html_comments_pattern, '', $markup);
	return !empty(_kiso_strip_tags($cleaned_region_output, $allowed_tags));
	}
	/**
	* Strips html tags, except allowed, returning a trimmed clean markup.
	*/
	function _kiso_strip_tags(string $markup, string $allowed_tags = '') {
	$allowed_tags .= '<drupal-render-placeholder><img>';
	return trim(strip_tags($markup, $allowed_tags));
	}

Could you please provide me with code example to implement in the twig template?

[lramarojaona]

@smillart ,
I implemented the suggested way and removed related PHP code.

I did not add the strip_tags, I guess we'll need to add it somehow to prevent breaking existing sites.

So this patch is WIP.

kiso-64.patch

FYI, without this, my region is not outputed. Using Openfed 12.1.1

[lramarojaona]

There is a small typo in previous patch with page_foote which should be page_footer.

Here's the updated patch.

kiso-64-2.patch

[GyD]

@lramarojaona I confirm you need to add |striptags('<drupal-render-placeholder><img><video><[...]>')|trim in order to have it work. Otherwise you end up with empty region displayed.

Maybe using https://www.drupal.org/project/twig_real_content could be a good idea if bosa agree!?

[lramarojaona]

Ok, maybe we should create custom twig functions or filters for that: https://drupaljournal.com/article/drupal/creating-custom-twig-functions-and-filters-drupal

[rutiolma]

I don't fully agree with the solution. Removing this twig function will require an update to hundreds of sites, which is not really super simple.
Why can't we keep the old function and adapt it to use real content?! Wouldn't that be more efficient?

[lramarojaona]

@rutiolma , good point, but it should be somehow marked as deprecated.

[rutiolma]

@rutiolma , good point, but it should be somehow marked as deprecated.

IMHO we could keep the "has_region" function in Kiso and use it as we've been doing. It's way more clean to check for an empty region.
This
{% if has_tools %}
vs
{% if page.tools|render is real_content %}

If we mark it as deprecated, we should remove it for version 4.0.x and not before otherwise all existing projects will have to change their themes due to a minor release.

[jclemmenb4y]

I seems to be a safer approach for now.

I know I'm overriding the has_region function in almost all my themes as it was not working properly (especially for regions with content such as facets).

But does this approach solve anything performance wise? As this was the initial concern. @GyD what do you think?

[GyD]

We should be able to create a has_{region} using php classes from twig_real_content within _kiso_has_region function. I’ll investigate that.

edit : that’s what @rutiolma is doing in Feature/#64 in fact.

[ninjadrupal]

To give more detail <?php render function do not return the final output. That could lead to false result, example: when a drupal placeholder return nothing.

This can be tested with facets module and facet option that do not render the facets when the facet source is not rendered.

Laurent solution is the one we put on werkenvoor but may not be the best since this issue need investigation.

Example, with facet we currently have this output in php on page where facet are not rendered:

'  <drupal-render-placeholder callback="Drupal\\block\\BlockViewBuilder::lazyBuilder" arguments="0=category&amp;1=full&amp;2" token="3H-ATSLp_IKUKYnhOjRX3yjcYQjgLT6WOepB79hscJI"></drupal-render-placeholder>
'

But when you render in twig it return

''

Please note that solution implemented in Kiso also render region and their content multiples times (at least twice), that could lead to performances issues.

i have the same issue in one of the sites :/ after the upgrade to openfed 12