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

Accounts for non-array fieldId in MatrixBlockQuery #12635

Closed
wants to merge 1 commit into from

Conversation

markhuot
Copy link
Contributor

@markhuot markhuot commented Feb 6, 2023

During a Craft 3.7 -> Craft 4.3 upgrade I ended up with a $this->fieldId=69 which is not a foreach-able array. The fix was to add the fieldId as a tag directly, but I'm not sure if that affects other parts of the code?

The big question: is this a result of a bad upgrade on my part or an actual bug? The type hints seem to imply that an int is a valid value for ->fieldId but I'm not sure if they're just out of date?

@markhuot markhuot requested a review from a team as a code owner February 6, 2023 18:44
@brandonkelly
Copy link
Member

brandonkelly commented Feb 7, 2023

fieldId should be getting normalized to an array or false via _normalizeFieldId() (called at the beginning of beforePrepare(). Are you doing something to prevent that from getting called?

@markhuot
Copy link
Contributor Author

markhuot commented Feb 7, 2023

No, I get this when clicking "New entry" on the /admin/entries screen of a 4.3.6.1 install.

Here's the error I get when I click the button which triggers a POST to index.php?p=admin%2Factions%2Fentries%2Fcreate&v=1675795476250

Screenshot 2023-02-07 at 1 44 55 PM

@brandonkelly
Copy link
Member

Can you post the full stack trace?

@markhuot
Copy link
Contributor Author

markhuot commented Feb 14, 2023

I can do you one better. I can recreate this on a vanilla install of Craft 4.3.8.2 by creating a single section with a single matrix block with a single plain text field with a handle of cacheTags. Doesn't matter what you name the section, the block type, etc… if there's a matrix field with a handle of cacheTags it throws Craft for a loop.

image

The bug is that when you click to create a new entry in the CP it calls ->toArray() on that new entry (after generating it). The ->toArray() method then calls ->resolveFields() on the entry (via Yii's Arrayable trait) and tries to serialize all the fields of the entry. Eventually it reaches the matrix field which kicks off a recursive ->toArray() call on the matrix block query. The matrix block query then calls its own ->resolveFields() which sees the cacheTags handle and tries to serialize the cacheTags field. Unfortunately MatrixBlockQuery has its own getCacheTags getter which gets called instead of our field because Yii prioritizes getters over behaviors (which is where the custom field behavior should have kicked in).

I'm not sure of the best fix for this. You could blacklist the cacheTags handle but that seems heavy handed. Ideally the custom fields would be queried directly via the behavior instead of the magic __get?

Edit: thinking about this a little bit more: I guess the prepare methods never run in the ->toArray() flow. So maybe another fix would be to prepare element queries before they are serialized to an array?

@brandonkelly
Copy link
Member

Thanks! I fixed this within ElementQuery::fields() so toArray() will explicitly pull custom field values from the behavior, rather than giving the magic methods a chance to interfere.

Craft 3.7.66 and 4.3.9 are both out with the fix.

(Technically the root bug existed in Craft 3, even though it wasn’t causing any problems there.)

@markhuot
Copy link
Contributor Author

🎉 this is fantastic thanks! Will get it checked out and tested later today and confirm everything is good.

@markhuot markhuot deleted the patch-3 branch February 14, 2023 22:10
@markhuot
Copy link
Contributor Author

This is working great in our dev environment! Thank you.

@brandonkelly
Copy link
Member

Great to hear!

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

Successfully merging this pull request may close these issues.

2 participants