Skip to content

Don't let custom field handles shadow element query properties - #19640

Open
markhuot wants to merge 1 commit into
craftcms:5.xfrom
markhuot:bugfix/criteria-attributes-query-property-collision
Open

markhuot wants to merge 1 commit into
craftcms:5.xfrom
markhuot:bugfix/criteria-attributes-query-property-collision

Conversation

@markhuot

@markhuot markhuot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Bit by an old legacy site that was created before where made it in to the blacklisted field names. Have a fix for it here, if you'd like… not sure if it's something you want to account for though.


On an install with a custom field whose handle collides with an ElementQuery property (ours is where), relational fields nested in Matrix fields fail validation with "should contain at least 1 selection" on rows that have a selection.

where is in RESERVED_HANDLES, but that's only validated on save, so fields created before the handle was reserved are still out there. Ours is a Redactor field on an unrelated entry type.

Repro

Custom field handled where. Matrix field whose entry type has an Entries subfield with minRelations of 1. Two or more Matrix entries, each related to a different entry. Save the owner.

Every row but the first fails. Point them all at the same entry and it passes.

Debugging

criteriaAttributes() adds behavior properties without checking whether the handle collides with a property the query already has. On collision the query property wins on read, so getCriteria() returns the SQL condition:

row 225 getCriteria() -> where={"elements.id":[115]}
row 226 getCriteria() -> where={"elements.id":[135]}

PLAN handle=...:singleProduction count=true criteria.where={"elements.id":[115]}

validateRelationCount() calls _all()->eagerly(), and count() routes into eagerLoad(), which passes $this->getCriteria() into the plan. Row 225's target restriction then constrains the shared target query for every sibling. eagerLoadElements() seeds each source to 0 and only overwrites non-zero counts, so the rest keep the 0, and count() returns it rather than falling back to a real query.

validateRelatedElements() has the same exposure (->eagerly() then ->all()): siblings come back empty and skip validation. Not fixed here.

Fix

!property_exists($this, $name) &&

Only the validation path is affected. eagerly is off by default, and ->all(), ->one(), and explicit with() eager loading were all already correct. Colliding handles also drop out of getCriteria(), but they were never usable as params anyway.

Two tests in ElementQueryTest, both failing without the fix. Full unit suite passes.

Separately: select, from, join, having, params, orderBy, limit, offset, groupBy, union, distinct, and indexBy are all public query properties and none are in RESERVED_HANDLES.

A field handle that collides with one of ElementQuery's own properties
(e.g. `where`) was still being added to criteriaAttributes(), so
getCriteria() returned internal query state rather than a field value.

That leaked one element's relation constraint into the shared
EagerLoadPlan built by ElementQuery::eagerLoad(), which made relational
fields nested within Matrix fields fail count validation for every
sibling element except the first.
Copilot AI lite review requested due to automatic review settings September 15, 2026 17:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

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