NEW: Allow for user-created objects to have values passed in the constructor #8591
Conversation
sminnee
changed the title
I might actually revisit this — it looks like both original and changed should reflect the content currently in the database, which in the case of new records should have none of the content derived from populateDefaults(). However, I'll get the tests passing without touching this before diving into that, and I'll see if there are tests that rely on this quirky behaviour, and if so, why... |
There was a problem hiding this comment.
Seems like a nice addition to me. There's a stray var_dump and some tests would be nice for this - especially some that confirm the existing isSingleton functionality asserting there's appropriate BC although that LGTM.
I might have a go at adding some later. I feel like I need to be more familiar with the ORM - but don't wait for it!
| $item = Injector::inst()->create($class, $row, false, $this->getQueryParams()); | ||
| $creationType = empty($row['ID']) ? DataObject::CREATE_OBJECT : DataObject::CREATE_HYDRATED; | ||
|
|
||
| $item = Injector::inst()->create($class, $row, $creationType, $this->getQueryParams()); |
There was a problem hiding this comment.
Couldn't this just be in DataObject::__construct, and check the creation type from $row? I'm not sure why the tri-state needs to be outside of the DataObject. The existing (singleton / prototype) seemed sufficient as an external API for all objects.
I'm worried about the tri-state for dataobjects, bool state for all other objects and coupling of injector to dataobject mostly.
There was a problem hiding this comment.
I'm worried about the tri-state for dataobjects, bool state for all other objects and coupling of injector to dataobject mostly.
AFAIK DataObject is the only class that makes use of this 2nd argument, so what we're talking about here is turning a bi-state argument tri-state argument.
Hyrdation, Creation, and Singletons are different cases. In the status-quo code, Creation and Hydration are coupled with the assumption that the Creation case will have an empty record. Frankly, Hydration and Singleton have more in common so you could argue "maybe hydration should set isSingleton = true". But I think that this will risk more messy edge-cases and also risk semver breakages. So I think a tri-state field is better.
We could arguably use the presence of an ID value to distinguish hydration (ID is present) from creation (ID isn't present), but that will cause bugs in two cases:
- We attempt to hydrate a record without an ID
- We attempt to create a record with a predefined ID
The former case seems unlikely unless we change our ORM, but the latter could conceivably occur in edge-cases such as copying content from stage to draft. I can't guarantee that it will introduce bugs, but it seems a risky proposition — I think a tri-state is cleaner.
Why is this conditional needed?
This condition was added to make ManyManyThroughList::add() work, which creates a new object by calling $hasManyList->createDataObject() on line 217.
So, if we wanted to reduce the use of the conditional here, we could potentially deprecate createDataObject() in favour of hydrateDataObject() and createNewDataObject(). I'd probably still leave this code as-is, but at least the messiness would be contain in a deprecated API.
There was a problem hiding this comment.
I agree with Sam, in that it's safer to make this an explicit decision of the API caller (tri-state), rather than keep the current signature and infer from the various constructor argument combinatoins (bi-state)
|
Damian gave some feedback here that I don't really think should be incorporated into this PR. I feel it pushes towards more like "possible bigger refactor in SS5", when here I have an SS4-appropriate fix. There's one suggestion I had for partially addressing his concern:
Should I do this? If I do that, shall we merge it? Cc @dhensby @chillu @kinglozzer @tractorcow for good measure. |
| $item = Injector::inst()->create($class, $row, false, $this->getQueryParams()); | ||
| $creationType = empty($row['ID']) ? DataObject::CREATE_OBJECT : DataObject::CREATE_HYDRATED; | ||
|
|
||
| $item = Injector::inst()->create($class, $row, $creationType, $this->getQueryParams()); |
There was a problem hiding this comment.
I agree with Sam, in that it's safer to make this an explicit decision of the API caller (tri-state), rather than keep the current signature and infer from the various constructor argument combinatoins (bi-state)
|
Cool I’ll address feedback on hackday. |
|
Ok I've squashed the 3 agreed bits of feedback into the
Should be good to merge now? |
|
OK got a few weird failures here I think when I rebased... a couple of fixes dropping in now. |
|
A side-effect of this change is that NULL column values are now included in toMap(), whereas previously they were excluded. This is because the code previously had an "if !== null" check when populating $this->record. I can't tell where this is a bugfix or a breaking change, although it has caused some issues with calculated field comparison when assertListEquals is passed the result of toMap() for a DataObject with calculated fields. In particular:
So, what's our conclusion? Should toMap() exclude NULL fields (the current behaviour on @silverstripe/core-team is the restoration of NULL values to toMap() results a bugfix or an API change? |
Comparing every single field is unnecessary and brittle, only the IDs need to be compared. Notably this tripped over a potential bug fix in silverstripe/silverstripe-framework#8591 but the change should be incorporated regardless.
|
The CMS PR silverstripe/silverstripe-cms#2441 will fix the failing test on this, but it would be good to confirm whether what I've fixed is, in fact, an API change before merging this. |
|
What was the behaviour in SilverStripe 3? I think having null values present would be making the API honest about its intention, but I also recognise that it might break someone's code somewhere. |
I've looked at |
|
Would it be silly to suggest we change it but also provide a configuration option to revert back if people find it breaks something? |
I don't think this will help much. I think the hard thing will be identifying that this is what's happening; once it's identified it's pretty easy to fix. And config flags for such deep behaviour is likely to make the system less reliable overall. If we do change it, we should list it in the upgrade notes though. I might break this out to a separate bug card. |
|
If this breaks someone's code, the proper fix would probably be to add a null check in their loop, not tweak some random configuration flag that they will never have heard off If it's actually breaking our own existing test, it sounds like a sensible expectation that this will be breaking someone's code somewhere. So I would err on the side of not returning nulls. |
|
In my view it broke a test that abused assertListEquals by pushing the result of toMap into it, and https://github.com/silverstripe/silverstripe-cms/pull/2441/files shows what was the right way to do it. But, yeah, a bit of a value-judgement there. |
|
Broke out a separate bug card here #9021 |
|
Context for the test that broke:
|
|
Following up form my comment on #9021, I'm going to amend this PR to do the following:
|
|
@sminnee Do you have time to work on those amendments, or shall we close this out for the time being? |
sminnee
changed the title
sminnee
changed the title
|
Thanks for the nudge @Cheddam I've addressed the final work on this now |
…tructor. This change extends isSingleton into a tri-state creationType, which recognises a new option, CREATE_HYDRATED. This is used to create records from database content, which bypasses both setters and defaults. Which that in place, the default, non-hydrated call will use setters via DataObject::update(), and is therefore allowable for user-code instantiating new objects. Some quirks of existing behaviour are retained, notably that for new objects data created by populateDefaults() will register as a change in $this->original vs $this->record, but not $this->changed.
This was latent behaviour in the previous implementation; now it is explicit behaviour.
sminnee
dismissed stale reviews from chillu, ScopeyNZ, and maxime-rainville
Addressed feedback
|
okay this is ready for another review thanks |
This change extends isSingleton into a tri-state creationType, which
recognises a new option, CREATE_HYDRATED. This is used to create records
from database content, which bypasses both setters and defaults.
Which that in place, the default, non-hydrated call will use setters
via DataObject::update(), and is therefore allowable for user-code
instantiating new objects.
Some quirks of existing behaviour are retained, notably that for new
objects data created by populateDefaults() will register as a change in
$this->original vs $this->record, but not $this->changed.
Parent issue
To do