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

Repeated database queries from BaseElement::getAreaRelationName() #928

Closed
kinglozzer opened this issue Aug 17, 2021 · 5 comments
Closed

Comments

@kinglozzer
Copy link
Member

kinglozzer commented Aug 17, 2021

During element rendering, forTemplate() calls getRenderTemplates(), which in turn calls getAreaRelationName() to help build a list of applicable templates (presumably so that elements can be rendered differently based on which area they belong to).

BaseElement::getAreaRelationName() calls $this->Parent(), which runs through a typical flow for fetching a has_one component. However, these calls aren’t cached so this results in an additional database query for each element as it’s rendered:

if ($page) {
$has_one = $page->config()->get('has_one');
$area = $this->Parent();

I’d suggest we replace this with something like:

$class = DataObject::getSchema()->hasOneComponent($this, 'Parent');
$area = ($this->ParentID) ? DataObject::get_by_id($class, $this->ParentID) : null;
$area = $area ?: ElementalArea::singleton();

PR

@michalkleiner
Copy link
Contributor

Just a quick thought — wasn't there an enhancement from @mfendeksilverstripe around gettoppage or similar? Not sure if it applies here but it's the first thing that came to mind.

@mfendeksilverstripe
Copy link
Contributor

You're right @michalkleiner , we do have a feature addressing this. It's opt in in 4 and built in on master. See Top page feature for more details @kinglozzer.

@kinglozzer
Copy link
Member Author

Unless I’m misunderstanding something, I’m not sure that’d help in this case. The code in question has already fetched the page, it’s another query after that that then fetches an ElementalArea record that I’m trying to eliminate 😄

@mfendeksilverstripe
Copy link
Contributor

You're right @kinglozzer, the Top page feature is focused on page lookup from blocks. I think it's reasonable to add a cached call for elemental area as well.

kinglozzer added a commit to kinglozzer/silverstripe-elemental that referenced this issue Aug 21, 2023
kinglozzer added a commit to kinglozzer/silverstripe-elemental that referenced this issue Aug 23, 2023
GuySartorelli added a commit that referenced this issue Aug 23, 2023
FIX: Save repeated database queries to fetch elemental area name (fixes #928)
@GuySartorelli
Copy link
Member

Pull request merged. It'll be automatically tagged by GitHub Actions.

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

5 participants