Feature/preload singles

[brandonkelly]

Description

Ensures any Single entries are preloaded & memoized at the beginning of each template, if their handles are referenced as variables anywhere in the template.

Then, wherever their handles are referenced, the Single entries will be swapped in as fallback values, if and only if the variable is otherwise undefined at that point in the template.

For example, if you have a Single section with the handle footer and somewhere in your template you have:

{{ footer.copyrightNotice }}

then footer will be automatically available, even if the template didn’t explicitly fetch it.

However if the template did this:

{% set footer = null %}
{{ footer.copyrightNotice }}

you’d get a Twig error, because footer is null and does not include a copyrightNotice property.

Additionally, the Single entries will never touch the variable context. So {{ dump(_context) }} would not include the Single entry handles unless they had been explicitly defined by the template.

Related issues

• Preload Single entry variables #12698

[bencroker]

Will craft.app.elements.eagerLoadElements() continue to work with this?

[mmikkel]

From testing a little bit, it looks like the is defined test is still potentially problematic:

{% if contactInfo is defined %}
    ...
{% endif %}

In Craft 4.3, the above conditional will evaluate to false if there isn't a variable contactInfo defined in the template's context. But with this PR, it will evaluate to true even if there isn't a contactInfo variable in the context, but there happens to be a single with that same handle.

I assume this is an oversight in the new PR and that this potential BC can be mitigated.

To be completely honest though, I still feel like this feature just isn't a good idea overall, and especially not for a minor release. It feels like too big a change and I'm not confident that all bases are truly covered in terms of it not introducing (potentially silent) errors on a lot of sites.

Curious: What's the reason for not wanting to go with a "Preload" (or "Make global") section setting that was proposed in #12698? I appreciate the DX effort here, but from my POV this seems like a prime candidate for an opt-in approach; especially considering the fact that many sites won't have any use for the feature in the first place as entryfying globals is going to be a manual effort for the time being.

[brandonkelly]

@bencroker Yep, although there wouldn’t be any point in calling that for a single; you could just set with params when fetching individual Matrix/relational fields off of the single, for the same performance gain.

[brandonkelly]

From testing a little bit, it looks like the is defined test is still potentially problematic:

{% if contactInfo is defined %}

    ...

{% endif %}

In Craft 4.3, the above conditional will evaluate to false if there isn't a variable contactInfo defined in the template's context. But with this PR, it will evaluate to true even if there isn't a contactInfo variable in the context, but there happens to be a single with that same handle.

I assume this is an oversight in the new PR and that this potential BC can be mitigated.

That was intentional (is defined checks and normal variable references were each implemented separately), but I could be persuaded to remove the fallback for is defined checks.

To be completely honest though, I still feel like this feature just isn't a good idea overall, and especially not for a minor release. It feels like too big a change and I'm not confident that all bases are truly covered in terms of it not introducing (potentially silent) errors on a lot of sites.

Curious: What's the reason for not wanting to go with a "Preload" (or "Make global") section setting that was proposed in #12698? I appreciate the DX effort here, but from my POV this seems like a prime candidate for an opt-in approach; especially considering the fact that many sites won't have any use for the feature in the first place as entryfying globals is going to be a manual effort for the time being.

If it were opt-in, I imagine it would lead to support because developers won’t understand why some singles are immediately available to their templates, but others aren't.

I like this because it significantly improves the DX for working with singles, which is currently super awkward to me, in that you have to go through the whole craft.entries() thing just to get a single entry. This will drastically simplify templates while improving performance at the same time (by querying all referenced singles in one query and memorizing them for other templates). So to me, it seems like something we should be making as accessible as possible so long as we're not breaking templates (which I believe we’ve accomplished here).

[mmikkel]

That was intentional (is defined checks and normal variable references were each implemented separately), but I could be persuaded to remove the fallback for is defined checks.

It's potentially going to break existing templates either way, isn't it? Which way you go with it should probably be the option that carries the least risk of things breaking. Which I think this FR generally does a very good job with, but it's probably impossible to make it air-tight, hence why I think this isn't an appropriate change for a minor release, unless it's made opt-in.

If it were opt-in, I imagine it would lead to support because developers won’t understand why some singles are immediately available to their templates, but others aren't.

Maybe. Personally, I don't think that a "Preload" or "Make available globally" setting would be that much harder for developers to grok than, for example, the URI setting. It might even be helpful in putting emphasis on the feature, and making developers aware of how it works.

I like this because it significantly improves the DX for working with singles, which is currently super awkward to me, in that you have to go through the whole craft.entries() thing just to get a single entry. This will drastically simplify templates while improving performance at the same time

I appreciate that, but singles are created for all sorts of purposes.

For example, some singles that have URIs and serve as either listing pages (e.g. "News", "Projects", "Employees") or special one-off pages ("Contact", "Privacy Policy") are rarely needed outside of their own entry template. Being able to pull these entries in from anywhere via their handles isn't really going to be that helpful or simplify anything, especially not for existing sites built without this feature in mind.

For some singles, the preloading will definitely be a boon, and perhaps I'm overreacting here. I just think that, even if the concepts of singles and globals are merging, effectively starting to treat all singles as globals isn't going to be the best approach. It's great that this FR does a good job of avoiding variable conflicts, but personally I still struggle with the idea of having an entry potentially pulled in just because I happen to type something like news, projects or employees into a template (instead of the Variable "news" does not exist exception I might've otherwise had, and expected). In my opinion, it's going to add some extra cognitive overhead for - in many cases - little benefit.

[brandonkelly]

Discussed this internally and decided to toggle this behavior with a new preloadSingles config setting. It’s disabled by default in Craft, so there’s zero chance it will break existing projects automatically, but I’m going to have it enabled by default in the craftcms/craft starter for new projects, when 4.4 ships.

Given that you’ll be opting into these changes, I left the is defined check alone (so it will return true for single handles). Feels right considering the variable is defined, even if not in the context.

[mmikkel]

Great, I think that's a good compromise for the time being! Thanks Brandon.

[brandonkelly]

4.4.0-beta.5 is out with these changes!

[gglnx]

If I enable preloadSingles I get Twig compile errors in combination with ??:

ParseError: syntax error, unexpected token "," in storage/runtime/compiled_templates/5f/5f3fca272ae13b52fab44ef646855901.php:39
Stack trace:
#0 vendor/twig/twig/src/Environment.php(342): Twig\Cache\FilesystemCache->load('/Users/dennis/P...')
#1 vendor/twig/twig/src/Environment.php(309): Twig\Environment->loadTemplate('__TwigTemplate_...', 'twig-twig')
#2 vendor/twig/twig/src/Environment.php(277): Twig\Environment->load('twig-twig')
#3 vendor/craftcms/cms/src/web/View.php(451): Twig\Environment->render('twig-twig', Array)
#4 vendor/craftcms/cms/src/web/View.php(504): craft\web\View->renderTemplate('twig-twig', Array)
#5 vendor/craftcms/cms/src/web/TemplateResponseFormatter.php(56): craft\web\View->renderPageTemplate('twig-twig', Array, 'site')
#6 vendor/yiisoft/yii2/web/Response.php(1098): craft\web\TemplateResponseFormatter->format(Object(craft\web\Response))
#7 vendor/craftcms/cms/src/web/Response.php(286): yii\web\Response->prepare()
#8 vendor/yiisoft/yii2/web/Response.php(339): craft\web\Response->prepare()
#9 vendor/yiisoft/yii2/base/Application.php(390): yii\web\Response->send()
#10 web/index.php(14): yii\base\Application->run()
#11 {main}

Test Twig file:

{{ settings.test ?? 'test' }}

The complied template:

<?php

use Twig\Environment;
use Twig\Error\LoaderError;
use Twig\Error\RuntimeError;
use Twig\Extension\SandboxExtension;
use Twig\Markup;
use Twig\Sandbox\SecurityError;
use Twig\Sandbox\SecurityNotAllowedTagError;
use Twig\Sandbox\SecurityNotAllowedFilterError;
use Twig\Sandbox\SecurityNotAllowedFunctionError;
use Twig\Source;
use Twig\Template;

/* twig-twig */
class __TwigTemplate_97e5b84915c4e0c16c210a6a0d1f2c04 extends Template
{
    private $source;
    private $macros = [];

    public function __construct(Environment $env)
    {
        parent::__construct($env);

        $this->source = $this->getSourceContext();

        $this->parent = false;

        $this->blocks = [
        ];
    }

    protected function doDisplay(array $context, array $blocks = [])
    {
        $macros = $this->macros;
        craft\helpers\Template::beginProfile("template", "twig-twig");
        craft\helpers\Template::preloadSingles(['settings']);
        // line 1
        (((craft\helpers\Template::attribute($this->env, $this->source, (isset($context["settings"]) || array_key_exists("settings", $context) ? $context["settings"] : (craft\helpers\Template::fallbackExists("settings") ? craft\helpers\Template::fallback("settings") : null), "test", [], "any", true, true) &&  !(null === craft\helpers\Template::attribute($this->env, $this->source, (isset($context["settings"]) || array_key_exists("settings", $context) ? $context["settings"] : (craft\helpers\Template::fallbackExists("settings") ? craft\helpers\Template::fallback("settings") : (function () { throw new RuntimeError('Variable "settings" does not exist.', 1, $this->source); })())), "test", [])))) ? (print (twig_escape_filter($this->env, craft\helpers\Template::attribute($this->env, $this->source, (isset($context["settings"]) || array_key_exists("settings", $context) ? $context["settings"] : (craft\helpers\Template::fallbackExists("settings") ? craft\helpers\Template::fallback("settings") : (function () { throw new RuntimeError('Variable "settings" does not exist.', 1, $this->source); })())), "test", []), "html", null, true))) : (print ("test")));
        echo "
";
        craft\helpers\Template::endProfile("template", "twig-twig");
    }

    public function getTemplateName()
    {
        return "twig-twig";
    }

    public function getDebugInfo()
    {
        return array (  39 => 1,);
    }

    public function getSourceContext()
    {
        return new Source("{{ settings.test ?? 'test' }}
", "twig-twig", "src/templates/twig-twig.twig");
    }
}

[brandonkelly]

@gglnx Doh, thanks for reporting! Could have sworn I tested that 🤦🏻 Fixed now in 4.4.0-beta.6. You will need to clear your compiled template caches after updating, either from Utilities → Caches, or by running

php craft clear-caches/compiled-templates