servdhost / craft-asset-storage

MIT License
10 stars 1 forks source link

Context missing from `dynamicInclude` in multisite #60

Open chrismlusk opened 1 year ago

chrismlusk commented 1 year ago

I'm having issues with dynamically including a template in a multisite setup.

Take the following example, which is being used in a for block in blocks loop:

{% dynamicInclude "_blocks/productList.twig" with {block: block} only placeholder %}
    {% include "_partials/product-list-skeleton.twig" %}
{% endDynamicInclude %}

In the primary site, everything works great. In my second site, however, my block variable is missing from the context. The ?p=actions/servd-asset-storage/dynamic-content/get-content request comes back with the following Twig runtime error:

Impossible to access an attribute ("headline") on a null variable.

And that's because within my productList.twig template, I'm calling block.headline.

While investigating the issue, I tried replacing the dynamic include with a default include, and everything worked fine on my second site.

Versions

chrismlusk commented 1 year ago

@mattgrayisok this issue is still happening, but one update is that this seems to only be a problem in local development. The dynamically included template loads just fine on staging and production.

Also, I have other dynamically included bits of UI that work just fine locally in this second site, but these are elements that do not have a block variable, i.e., they are not the result of a Neo field query.

For reference, I'm doing something like:

{% set pageContent = entry.pageContent.all() %}

{% for block in pageContent %}
    {% include [
        "_blocks/_dynamic/" ~ block.type,
        "_blocks/" ~ block.type,
    ] ignore missing with {block: block} only %}
{% endfor %}

Take an example block type of myBlock that should be dynamically loaded. There is a file with that name in both:

The dynamic file looks like:

{% dynamicInclude "_blocks/myBlock.twig" with {block: block} only placeholder %}
    {% include "my-block-skeleton.twig" %}
{% endDynamicInclude %}

To test that it's not just an issue with this one particular block, I created dynamic versions of a handful of different blocks, and all of them fail the same way: The block variable is missing from the context.

And again, this is only happening in this second site. Everything (even the above tests I tried) works as expected in the default site.

Let me know if there is any other detail I can track down to help here.

mattgrayisok commented 1 year ago

Hi @chrismlusk

I've just been trying to recreate this but haven't had any luck yet. It's odd that it isn't working in local dev but is working in staging/prod - I'd imagine the code running in both should be the same.

Could you try running in local with Dev Mode disabled as that's the only thing I can think of that might change behavior between the two?

I have one other potential theory but debugging will involve editing some files in vendor so we'd best check the dev mode thing first!

chrismlusk commented 1 year ago

So, with Dev Mode disabled locally it sort of works. The get-content action responds with markup and no errors, but it's clear my block variable is still missing because the generated markup is incomplete.

My template:

<h2>{{ block.headline }}</h2>

The output with Dev Mode disabled:

<h2></h2>
mattgrayisok commented 1 year ago

Let's edit some vendor files then!

You should have a file at /vendor/servd/craft-asset-storage/src/StaticCache/Twig/IncludeNode.php which contains the logic which is used for finding the context for the dynamicInclude and encoding it.

For reference, the file in this repo is:

https://github.com/servdhost/craft-asset-storage/blob/craft-4/src/StaticCache/Twig/IncludeNode.php

We're interested in the final function cleanContextArray which takes the context and removes anything that we can't cleanly encode in an ajax request. My theory is that the block variable is being removed in here.

We can test that by adding a few LOC at the very bottom of the foreach ($a as $key => $el) { loop in there, giving us something like:

foreach ($a as $key => $el) {
    // Existing if()s can be left alone

    if($key == 'block'){
        var_dump($key);
        var_dump(gettype($el));
        if(gettype($el) == "object"){
          var_dump(get_class($el));
        }
        exit;
    }
}

That should cause the block variable to get dumped out when you load the page (but only if the Servd plugin has chosen to strip it from the context) along with its type. We can then determine if the type is something we need to include handling for in the plugin, or if something else weird is going on.

chrismlusk commented 1 year ago

@mattgrayisok, OK I tried that and ... nothing happened. So, I started moving that debug conditional around, and when I place it within the if looking for objects that are subclasses of Craft's Element, I get a hit.

chrismlusk commented 1 year ago

I kept poking around in the vendor files while following the stack trace I'm seeing, and I found myself in controllers/DynamicContentController.php.

In the rehydrateArgs method, when getting the element by ID, that is returning null when I'm in the secondary site. When I go to my primary site, that returns the Neo block as expected.

So, I double checked the settings for that Neo field. The propagation method is set to Only save blocks to the site they were create in, and when I dump the arguments the DynamicContentController is rehydrating, I can confirm that the $el['id'] is different when I check the primary and secondary sites.

mattgrayisok commented 1 year ago

Thanks for that. We're definitely getting closer.

The getElementById function in the Craft codebase accepts a siteId param which I think is what we're missing here:

https://github.com/craftcms/cms/blob/bd601e32a1cf0ee5c96e7326db027887ea1b7ac9/src/services/Elements.php#L778

Since Elements have a unique ID, I had assumed that you could just pull them by ID, regardless of which site they belonged to, but perhaps the unique key for elements is actually ID + SiteId, in which case we need to account for that.

I'll get a test set up to recreate and then do... something... to sort that out!

mattgrayisok commented 1 year ago

I've just been trying to recreate with a setup that's pretty much identical to your own and still having no luck. I also had a look over the Craft source to see if that siteId is required and it doesn't seem to be - it just acts as an optional filter.

Can you confirm whether you're still only seeing the problem in local and staging/production are ok? If so I'm inclined to think that it might be some data corruption in the local database around the neo blocks. We should be able to test that by downloading a database dump from a working staging/production env and importing into local and seeing if the problem persists.

chrismlusk commented 1 year ago

Ah, I might have spoke too soon earlier when I said it was working in production/staging. Like I said in a previous comment, it only sort of works.

For reference, this block renders a template that includes an element query. In the CMS, editors can configure filters that are passed into the query, but the null coalescing operator sets a default so the block can still render with results.

{% set filters = block.filters.all() ?? ['*'] %}

Turns out, the block in question had no filters applied, so everything seemed as if it was working, but when I set filters, the output is an unfiltered list of results — so I assume block variable is missing and the fallback kicks in to keep things moving along.

However, I'm getting different results in production and staging 😬

Here's what I'm seeing in the secondary site for each environment:

Production

The dynamic include renders the block markup, but the block.headline is missing and the query results are not filtered as expected.

Staging

Everything works as expected for both sites.

Local

Nothing renders from the dynamic include (the placeholder is still there).

chrismlusk commented 1 year ago

I'm inclined to think that it might be some data corruption in the local database around the neo blocks. We should be able to test that by downloading a database dump from a working staging/production env and importing into local and seeing if the problem persists.

I've replaced my local DB with one from production and staging, but the problem persists.

chrismlusk commented 1 year ago

I thought of another difference between production and staging. Not sure if it's meaningful, but...

On production there is only one dynamic include on the page (this problem block).

On staging there are two. This block, and a debug utility we have to test geolocation features in staging.