Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Toll-free-bridge to DataObject::get_one() #637

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

jaredkipe commented Jul 12, 2012

There were three main ways to retrieve DataObjects back in SS 2.4
DataObject::get('ClassName') which was customizable and basically replaced with ClassName::get().
DataObject::get_by_id('ClassName') which was simple and cached, replaced by ClassName::get()->byID().

Then comes DataObject::get_one('ClassName') which was inherently cached and somewhat replaced by ClassName::get()->first(). But first() obscures the fact that it is basically a separate limit(1) query and if it is cached it certainly isn't yet.

Though I think it might be better to rewrite first() and last() to use a cache, here is a direct alternative to DataObject::get_one('ClassName') with the easy to remember and use format ClassName::get()->one();

jaredkipe added some commits Jul 12, 2012

Added DataList::one() to form a (partially) toll-free-bridge to DataO…
…bject::get_one(). Facilitates the use of the DataObject::$cache_get_one.
Contributor

jaredkipe commented Jul 12, 2012

Also, as a sanity check, I tested the performance of subsequent calls. As expected the first() and one(false) calls have basically the same operating time and make new MySQL calls each time. Subsequent calls to one() and one(true) are roughly 10% the time to call first().

Subsequent runs of first() one(false) one(true).
[12-Jul-2012 02:59:53] total = 100. max = 0.2524061203. average = 0.128145062923. min = 0.00257205963135. totalTime = 12.8145062923
[12-Jul-2012 03:00:06] total = 100. max = 0.27765083313. average = 0.140565447807. min = 0.00291085243225. totalTime = 14.0565447807
[12-Jul-2012 03:00:20] total = 100. max = 0.0315098762512. average = 0.0171865296364. min = 0.00284886360168. totalTime = 1.71865296364
[12-Jul-2012 03:01:20] total = 10. max = 0.0253939628601. average = 0.0140588760376. min = 0.00264096260071. totalTime = 0.140588760376
[12-Jul-2012 03:01:29] total = 10. max = 0.0273599624634. average = 0.0150725126266. min = 0.00287985801697. totalTime = 0.150725126266
[12-Jul-2012 03:01:36] total = 10. max = 0.00551390647888. average = 0.00420067310333. min = 0.00286793708801. totalTime = 0.0420067310333

Contributor

jaredkipe commented Jul 12, 2012

The only problem with just using DataObject::get_one() is that you lose all of the filter() and order() syntax. In fact I originally thought DataObject::get_one() was deprecated because of the change to how the $where field is parsed when DataObject::get_one() calls BACK into DataList.

DataObject already has hooks in to invalidate the cache on save etc. which seem to make it ideal for storing the cache (my thinking at roughly midnight last night).

Owner

chillu commented Aug 16, 2012

See discussion at https://groups.google.com/forum/?fromgroups#!topic/silverstripe-dev/NyzXquIEP-8%5B1-25%5D
I've suggested your way of fixing it as one possible option.

@sminnee sminnee closed this Aug 17, 2012

@sminnee sminnee reopened this Aug 17, 2012

This pull request passes (merged f7d2c7c into c91e855).

Contributor

jaredkipe commented Aug 17, 2012

I'm pretty confused as to what just happened here. What does 'passes' mean? My code doesn't seem to be in any of the main branches here on github.

Owner

chillu commented Aug 17, 2012

We're just trying out travis for automated checks of any pull requests: http://about.travis-ci.org/blog/pull-request-testing-for-everyone/
If it works out we'll update the contrib guidelines at doc.silverstripe.org/sapphire/en/misc/contributing
The close->reopened step from sminee was unrelated to that from what I can tell. travisbot does its thing automatically, it merges into a temporary branch and runs unit tests, but doesn't *actually" merge the pull request.

Owner

sminnee commented Aug 17, 2012

Travis was just set up; the close/reopen was a way to trigger the build manually for existing pull requests.

@chillu chillu referenced this pull request in silverstripe/silverstripe-cms Aug 28, 2013

Merged

BUG Fixed up SQL table.column identifiers, SQL encoding issue #802

@simonwelsh simonwelsh added the master label Mar 14, 2014

Update Controller.php for extending handleAction()
Controller's superclass (RequestHandler) has some extend() events in its handleAction() that are avoided by Controller's implementation of handleAction().
Owner

halkyon commented Sep 24, 2014

I don't think this solves the caching issue entirely for DataList. Given the discussion in the mailing list trailed off, and this pull request is really old and is now showing an unrelated commit, I'm going to close it.

@halkyon halkyon closed this Sep 24, 2014

Contributor

jaredkipe commented Sep 24, 2014

I don't know why GitHub will just include new commits into existing pull requests. I guess they really want to push the idea of branching....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment