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

Refactor prepopulation #8395

Closed
wants to merge 4 commits into from
Closed

Refactor prepopulation #8395

wants to merge 4 commits into from

Conversation

sminnee
Copy link
Member

@sminnee sminnee commented Sep 21, 2018

NEW: Add Hierarchy::prepopulateTreeDataCache()

This provides a more extensible way of preopulating caches for optimised
tree generation.

Fixes #8391

Sam Minnee and others added 4 commits September 21, 2018 17:57
This helper is necessary when you want to replace a parameter with a
non-literal such as a column name. This arises when you are
transforming queries such as we do in the prepopulation query for
Hierarchy::numChildren()
API: Hierarchy::stageChildren() customisations must be applied to the base class and not include record-specific behaviour.

Adds the ability to prepopulate the cache for Hierarchy::numChildren()
in a batch.

Note that this optimisation means that stageChildren() is not called on
each record in order to calculate numChildren(). This means that the
structure of the stageChildren() query must be the same for all records
and the behaviour cannot be customised only for a subclass of the base
data class. For example, apply your customisations to SiteTree and not
a subclass.

This is an useful part of optimising the query count on tree generation.
See #8379
This will avoid trying to count the children of childless object.
This provides a more extensible way of preopulating caches for optimised
tree generation.

Fixes #8391
Copy link
Contributor

@ScopeyNZ ScopeyNZ left a comment

Choose a reason for hiding this comment

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

I feel like we should comment that this new replace_parameter function should be used with extreme caution. preg_matching out identifiers with literals is just a huge opportunity for insecure coding...

*/
public static function replace_parameter($sql, $paramIdx, $replacement, $skipEscaping = false)
{
$segments = preg_split('/\?/', $sql);
Copy link
Contributor

Choose a reason for hiding this comment

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

explode would be faster here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed on parent PR by 07f9cdc

src/ORM/DB.php Show resolved Hide resolved
@maxime-rainville
Copy link
Contributor

maxime-rainville commented Sep 23, 2018

@ScopeyNZ Parts of this PR are built on top of #8380. When #8380 has been merged, I'll have to rebase this one against it.

I'll take your feedback and implement it into the other one.

@sminnee sminnee closed this Sep 24, 2018
@sminnee sminnee deleted the refactor-prepopulation branch September 24, 2018 04:55
@sminnee
Copy link
Member Author

sminnee commented Sep 24, 2018

I've incorporated this into #8380

ScopeyNZ pushed a commit to creative-commoners/silverstripe-fluent that referenced this pull request Sep 25, 2018
maxime-rainville pushed a commit to silverstripe/silverstripe-cms that referenced this pull request Sep 25, 2018
* FIX: Use Hierarchy::prepopulate_numchildren_cache in tree-generation

Only relevant if silverstripe/silverstripe-framework#8380 is avialable,
however coded defensively so it can be merged before that PR if needs 
be.

See silverstripe/silverstripe-framework#8379

* FIX: Use Hierarchy::prepopulateTreeDataCache() in CMS.

Requires silverstripe/silverstripe-framework#8395

* Cache tree_class instead of assuming it will always be SiteTree.
maxime-rainville pushed a commit to silverstripe/silverstripe-versioned that referenced this pull request Sep 25, 2018
* FIX: Use Hierarchy::prepopulateTreeDataCache() in CMS.

Requires silverstripe/silverstripe-framework#8395
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.

3 participants