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

Add ability for nested element's to specify their owner's element type #16133

Open
wants to merge 2 commits into
base: 5.6
Choose a base branch
from

Conversation

nfourtythree
Copy link
Contributor

Description

In the NestedElementTrait the implementations of getOwner() and getPrimaryOwner() have a call to Elements::getElementById() currently this is hard-coded to pass null to the element type. This makes sense as there is no knowledge in this class of what the owner's element type would be.

Unfortunately, in certain scenarios this can cause an n+1 query issue with many duplicated (get element type) queries.

This PR adds the $ownerElementType property to trait. This property is useful for those instances where the nested element already knows what its owner's element type is. This is applicable in the case of variants in Craft Commerce.

In Commerce the getOwner() and getPrimary() methods from the trait have been overridden so that the element type can be specified (this greatly improved performance). With this PR this code can be removed and replaced with an implementation of the new property:

public ?string $ownerElementType = Product::class;

There was consideration to note adding the property but instead add an argument to both the getOwner() and getPrimaryOwner() methods as this could make things more flexible. But this felt like more of a change and greater scope for it to be a breaking change (based on developers who may have implemented/overridden the methods already).

@nfourtythree nfourtythree self-assigned this Nov 18, 2024
[ci skip]
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.

1 participant