search

spicywebau / craft-neo

A Matrix-like field type for Craft CMS that uses existing fields
Other
403 stars 63 forks source link

Neo block queries return duplicate child blocks #873

Closed chrismlusk closed 3 months ago

chrismlusk commented 3 months ago

Bug Description

Front-end queries are outputting grandchild blocks twice in a row. Everything appears normal when editing an entry.

I've experienced this from time to time for a few years on a particular project, and I have followed multiple issues filed here and in the Craft repo that describe seemingly related problems — many of which are hard to reproduce or have been addressed. Because I haven't been able to add any new detail to what is happening, I just resolve the issue by deleting the parent block and re-adding the content.

However, while working on a new project, I might have narrowed down the variables that are contributing to the problem.

The child blocks in this structure have no fields in their field layout — they are used to create limitations for which type of grandchildren are allowed. I find this helpful when a design renders the same layout — but authors need guardrails that enforce which type of data is allowed to be selected.

~So, when the duplication on the front end happens, these are the blocks where this happens.~

~I have plenty of other blocks with a more simple parent-child structure:~

~I have not had any duplication issues with these blocks.~

Any help debugging would be greatly appreciated! Let me know if sending any files or more detail would be helpful.

Steps to reproduce

Unfortunately I can't find a way to reliably reproduce this. What I have noticed is that this has happened when updating propagation method or changing an entry's status in one site but not another.

Neo version

4.0.8

Craft CMS version

4.8.6

What is the affected Neo field's propagation method?

Only save to site created in

Does this issue involve templating, and if so, is eager-loading used?

This is not a templating issue

chrismlusk commented 3 months ago

Well, maybe I spoke too soon. This just happened again, but the duplicates are from level two blocks. No idea what's leading to this.

Two days ago, when the site was last edited, everything was rendering just fine. Today, blocks are duplicated. The entry has not been edited, and nothing has been deployed (so, no settings changes).

chrismlusk commented 3 months ago

OK, after some more digging, I think this is an eager-loading issue — and it's probably my fault.

I'm working with a large page-builder field, and while I do follow Neo's instructions for eager loading, I believe the issue is that I do not eager load everything at the top level.

My code:

{% set blocks = entry.neoField.with([
    'blockTypeHandle:children.childBlockTypeHandle:children.childBlockTypeHandle:fieldHandle',
    ...
]).collect() %}

{% for block in blocks %}
    {% do block.useMemoized(blocks) %}
{% endfor %}

{% for block in blocks %}
    {% if block.level == 1 %}
        ...
    {% endif %}
{% endfor %}

I avoid eager loading every possible thing because there are plenty of blocks that are one-offs or rarely used. I only add eager loading at this top level for common blocks.

But then, in the templates of the one-off blocks that are not eager loaded, I'll do something like:

{% set grid = block.children
    .with([
        'grid2Up:children.demo:demoEntry',
        'grid4Up:children.demo:demoEntry',
    ])
    .collect().first() %}

In this block template in the project with the duplicate blocks, when I remove the .with method, the query returns the correct number.

What I find odd, however, is that this isn't always the case. Like I mentioned in the original issue, this only seems to periodically happen.

In fact, after looking again, the duplication does happen on blocks that are eager loaded at the top level, too.

Any ideas what I'm getting wrong? Or have I stumbled my way into a legitimate bug?

ttempleton commented 3 months ago

No need to do e.g. 'grid2Up:children.demo:demoEntry' - just 'demo:demoEntry' is all you need.

Does the block query work as expected if you simplify that?

chrismlusk commented 3 months ago

It works in that the correct number of blocks are rendered, but the demoEntry field is no longer eager loaded.

Also, in this example, grid2Up is a child block. Since Neo eager loading works like Matrix fields, I assumed that would need to be included?

ttempleton commented 3 months ago

Whoops, I misread that part of your comment, didn't realise it was a child block query. And that's probably not related if you're also seeing block duplication at the top level.

If possible, could you please send your composer files and database backup to plugins@spicyweb.com.au, along with information about an entry this is happening with (preferably at the top level if it's still happening there), and a template file or files containing a minimal reproduction of the block duplication on that entry, and we'll have a look.

chrismlusk commented 3 months ago

Thanks, I'll work on creating a minimal reproduction.

In the meantime, so I do not need to include children or the descendant block types in the eager loading path?

{% set blocks = entry.neoField.with([
    // Do this
    'grandchildBlockTypeHandle:fieldHandle',

    // Not this?
    'blockTypeHandle:children.childBlockTypeHandle:children.grandchildBlockTypeHandle:fieldHandle',
    ...
]).collect() %}

I removed the path of descendent block types, and I see that it does work.

But without children in the path, if you dump block.children from within one of the block templates, it shows that it's a BlockQuery. If the Neo field's content is correctly eager loaded, shouldn't that query be executed already?

ttempleton commented 3 months ago

block.children is a BlockQuery in that case, but initially getting every block in the field (not just the top level ones) and then executing the block.useMemoized(blocks) loop means that the block.children query will use the blocks instead of querying the database. So the child blocks have already been loaded from the database, even if the query for them is executed in a template in the same way as a non-eager-loaded/memoized query.

chrismlusk commented 3 months ago

Ah ok! That makes sense. I saw db/BlockQuery and assumed I was not doing something correctly 🤦🏼

So, it seems that me trying to optimize something that is already optimized has either created or stumbled on my original issue. Either way, removing children and the descendant block types from my eager loading paths reliably gets me the results I expect.

Thanks for the help!