BUG Fix DataObject / Versioned publishing issues #2917

Merged
merged 1 commit into from Mar 15, 2014

Conversation

Projects
None yet
3 participants
Contributor

tractorcow commented Mar 4, 2014

This issue was initially raised at and fixes silverstripe/silverstripe-blog#105.

After investigation I found the following issues.

If a DataObject was queried under a certain stage or reading mode, and then this mode changes, lazy-loading of fields would fail since it will always attempt to use the current mode. This is fixed in DataObject::loadLazyFields so that any DataObject will maintain it's own query parameters internally, propegating this context from initial load all the way to lazy loading of fields.

This fix was made in DataObject rather than in Versioned::augmentLoadLazyFields because (in my humble opinion) it's the role of the DataObject to maintain its own query context, and it should only be touched by extensions to modify this explicitly. This is easier than updating each dataobject extension across the board.

Secondly, Versioned::writeToStage was failing if no fields were modified, meaning simply loading a record from one stage and writing it to another would result in no write performed.

Test cases were included in VersionedTest rather than DataObjectLazyLoadingTest becuase I feel that this is more a test of the Versioned extension (and DataObject's support for extensions) than the lazy loading process itself. I could just as easy have put it there though. :)

I've marked this as "don't merge" because I'd like to get a few developer opinions before I'm confident this is the correct solution.

BUG Fix DataObject::loadLazyFields discarding original query parameters
BUG Fix Versioned::writeToStage failing to write object with unchanged fields
Contributor

tractorcow commented Mar 4, 2014

@sunnysideup would you like to peer-review my PR? :)

Contributor

sunnysideup commented Mar 4, 2014

Sure, what is the best way to do this? review the code or how it works or both etc...???

Contributor

tractorcow commented Mar 4, 2014

I just wonder if my thinking is correct in the getSourceQueryParams part of the code; If a dataobject is queried, is it wise to do a blind restore of the original query parameters when lazy loading?

Contributor

tractorcow commented Mar 5, 2014

Removed the "don't merge" - I think this can go into core as long as it's checked by another developer before they merge it.

Contributor

sunnysideup commented Mar 5, 2014

I just had a first look. It seems to make sense.

what if Query Params are already set, will it double-up or have too many query params? I am not sure if that is even relevant, but just wanted to check. I dont think this is an issue - and I think you are basically wondering about the same thing. I just wonder how I can even test this.

because it such a CORE CORE change, would it be worthwhile to work out how much longer it takes to run the code to avoid it slowing down EVERYTHING? This would be fairly easy to test and it is important to know.

Contributor

tractorcow commented Mar 6, 2014

Thanks for the review. :)

QueryParams are already set in the DataQuery constructor to the current defaults, such as versioned stage, etc. The code above simply overwrites any default keys with those saved on the DataObject, so it won't remove any keys. This might cause problems if any extension relies on the absence of any query param to work...

I don't think it's going to be a performance hit; It's just copying variables from one object into the DataQuery.

Contributor

simonwelsh commented Mar 15, 2014

I saw an old issue that this fixes too. I guess I'll have to go find it now :p

simonwelsh added a commit that referenced this pull request Mar 15, 2014

Merge pull request #2917 from tractorcow/pulls/fix-lazyload-queryparams
BUG Fix DataObject / Versioned publishing issues

@simonwelsh simonwelsh merged commit 90ba514 into silverstripe:3.1 Mar 15, 2014

1 check passed

default Scrutinizer: No new issues — Travis: Passed
Details

@tractorcow tractorcow deleted the tractorcow:pulls/fix-lazyload-queryparams branch Apr 6, 2014

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