Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.x]: merging arrays via twig filter drops custom fields #13009

Closed
therealpecus opened this issue Mar 29, 2023 · 5 comments
Closed

[4.x]: merging arrays via twig filter drops custom fields #13009

therealpecus opened this issue Mar 29, 2023 · 5 comments
Assignees

Comments

@therealpecus
Copy link

therealpecus commented Mar 29, 2023

What happened?

Description

When merging the results of an element query in a twig template with some data, custom fields are lost. The issue was originally identified with properties that were returned as collections (specifically, eager loaded elements).

Code that is migrated from Craft 3.x might suffer from this bug, and it might be quite puzzling since it's not documented in the migration path.

We use the merge pattern a lot in the main view templates to keep components as pure as possible (no Craft specific Twig extensions).

I believe collection objects passed to merge, map and reduce twig filters should be recursively converted to arrays in order to preserve the old behavior and this should be documented. Alternatively, these filters should be updated to preserve collections.

Steps to reproduce

  1. do an entry query with an eager loaded field {% set allNews = craft.entries().section('sectionNews').highlight('1').with([['thumbnailNews', 'tags']).one() %}
  2. merge some data {% set cards = allNews|merge({ extraField: 'demo' }) %}

Expected behavior

cards contains all fields from the query, specifically thumbnailNews and tags

Actual behavior

fields that were returned as collections are missing

Explanation

Craft CMS version

4.4.5

PHP version

8.1.x

Operating system and version

Linux (ddev)

Database type and version

n/a

Image driver and version

n/a

Installed plugins and versions

@brandonkelly
Copy link
Member

I compared how this works in Craft 3 vs. Craft 4. In Craft 4, all custom fields are excluded from the resulting array, not just eager-loaded fields.

Tracked it back to this commit, which was focused on excluding custom fields from elements’ getAttributes() method. It doesn’t look like it was intended for that to affect getIterator() as well though, which is called by Twig’s |merge filter (via iterator_to_array()).

So I’ve overridden getIterator() to bring custom fields back there (952d371). It’s a slight change in behavior so I made the change for Craft 4.5 rather than the next 4.4 release.

In the meantime, you can convert your entry to an array (non recursively) first, and then merge in your custom values:

{% set cards = allNews.toArray([], [], false)|merge({
  extraField: 'demo',
}) %}

@brandonkelly
Copy link
Member

Worth noting, there isn’t any performance benefit to eager-loading elements for a single source element. In fact, it’s going to result in a slight performance loss, as eager-loading does add a little overhead, so it should only be used when loading multiple elements up front.

@therealpecus
Copy link
Author

Thanks for the quick fix and workaround @brandonkelly ! I did notice later that all custom fields were dropped,

I had not thought of the performance overhead for single source element. Eager loading related elements has become a go-to pattern for me and I did not question it much for single queries.

Since this is going to be fixed, I don't think it makes sense to update the migration docs, so I'll amend the issue title to allow it to be found when searching for missing custom fields.

@therealpecus therealpecus changed the title [4.x]: merging arrays via twig filter drops keys that are collections [4.x]: merging arrays via twig filter drops custom fields Mar 31, 2023
@gbowne-quickbase
Copy link

I can confirm that this is now functioning in ^4.5.0-beta.1 the way it was in Craft 3.x. I just reverted the few lines I'd changed to use the {% do config(... %} methodology to the |merge( methodology and all the pages function as designed.

@brandonkelly
Copy link
Member

Craft 4.5.0 is out with that change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants