-
Notifications
You must be signed in to change notification settings - Fork 824
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
FIX Ensure getters and setters are respected #10708
Merged
emteknetnz
merged 1 commit into
silverstripe:5.0
from
creative-commoners:pulls/5.0/get-and-set
Mar 1, 2023
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
<?php | ||
|
||
namespace SilverStripe\ORM\Tests\DBFieldTest; | ||
|
||
use SilverStripe\Dev\TestOnly; | ||
use SilverStripe\ORM\DataObject; | ||
|
||
class TestDataObject extends DataObject implements TestOnly | ||
{ | ||
private static $table_name = 'DBFieldTest_TestDataObject'; | ||
|
||
private static $db = [ | ||
'Title' => TestDbField::class, | ||
'MyTestField' => TestDbField::class, | ||
]; | ||
|
||
public $setFieldCalledCount = 0; | ||
|
||
public function setField($fieldName, $val) | ||
{ | ||
$this->setFieldCalledCount++; | ||
return parent::setField($fieldName, $val); | ||
} | ||
|
||
public function setMyTestField($val) | ||
{ | ||
return $this->setField('MyTestField', strtolower($val)); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
<?php | ||
|
||
namespace SilverStripe\ORM\Tests\DBFieldTest; | ||
|
||
use SilverStripe\Core\Config\Config; | ||
use SilverStripe\Dev\TestOnly; | ||
use SilverStripe\ORM\DB; | ||
use SilverStripe\ORM\FieldType\DBField; | ||
|
||
class TestDbField extends DBField implements TestOnly | ||
{ | ||
public function requireField() | ||
{ | ||
// Basically the same as DBVarchar but we don't want to test with DBVarchar in case something | ||
// changes in that class eventually. | ||
$charset = Config::inst()->get(MySQLDatabase::class, 'charset'); | ||
$collation = Config::inst()->get(MySQLDatabase::class, 'collation'); | ||
|
||
$parts = [ | ||
'datatype' => 'varchar', | ||
'precision' => 255, | ||
'character set' => $charset, | ||
'collate' => $collation, | ||
'arrayValue' => $this->arrayValue | ||
]; | ||
|
||
$values = [ | ||
'type' => 'varchar', | ||
'parts' => $parts | ||
]; | ||
|
||
DB::require_field($this->tableName, $this->name, $values); | ||
} | ||
|
||
public $saveIntoCalledCount = 0; | ||
|
||
public function saveInto($dataObject) | ||
{ | ||
$this->saveIntoCalledCount++; | ||
return parent::saveInto($dataObject); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
<?php | ||
|
||
namespace SilverStripe\ORM\Tests\DataObjectTest; | ||
|
||
use SilverStripe\Dev\TestOnly; | ||
use SilverStripe\ORM\DataObject; | ||
|
||
class SettersAndGetters extends DataObject implements TestOnly | ||
{ | ||
private static $table_name = 'DataObjectTest_SettersAndGetters'; | ||
|
||
private static $db = [ | ||
'MyTestField' => 'Varchar(255)', | ||
]; | ||
|
||
public function setMyTestField($val) | ||
{ | ||
$this->setField('MyTestField', strtolower($val)); | ||
} | ||
|
||
public function getMyTestField() | ||
{ | ||
return strtoupper($this->getField('MyTestField')); | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need
$this->value->saveInto($dataObject)
as it will be called here if $val is a DBField.We should be using the arrow notation that calls __set() rather than calling __set() directly because there's some infinite recursion protection only when using arrow notation which I think happens at the PHP level, and also because calling __set() directly just looks weird.
Also update all the other fields that were changed in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shortcuts passed that, avoiding repeated calls through this chain when it's not needed. Imagine the (admittedly unlikely) scenario where a
DBField
instance has anotherDBField
instance as its value, which has anotherDBField
instance as its value, etc. What you're proposing will go through thesaveInto() => __set() => setField() => saveInto()
loop for every instance. The way I've got it, all of that is resolved insaveInto()
so that only the raw value gets passed through that series.The
if ($val instanceof DBField)
check is necessary for when we do$obj->MyField = new DBField()
i.e. set the dbfield instance to the value as though the value is a property. Imagine again that this newDBField
we're setting as the value has a series of nestedDBField
values - again, your suggestion results in it going through the loop a bunch of times. With the way I've got it, we just go__set() => setField() => saveInto()
at which point we resolve down to the raw value and only go down to__set()
one last time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean "the arrow notation"? Do you mean just pretend we're setting a property? If we do that, the tests fail.
The only way we'll run into that scenario is if a
DBField
has itself in the value chain, which is extremely unlikely. But if you like I can write some logic to detect that insaveInto
which wouldn't be difficult to do.That's not a good reason to not do the thing that preserves the expected behaviour. This is the only way to get the tests I've written passing, from what I can tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that will call __set(). We should be able safety assume the property doesn't exist on the DataObject since it's in the context of DBField->saveInto(), where $fieldName is DBField->name and Silverstripe ORM revolves around DataObject->FieldName magic.
I triggered it 2 different ways while investigating ways to simplify this PR. Chris's example code calls setField(), so does ViewableData::__set(), so it doesn't seem too hard, particularly when some related code gets modified in the future.
I'd rather take the small performance hit of repeated calls (we're talking microseconds) and ensure that we're calling all logic that should be called, rather than fragmenting the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it won't call
__set()
if we're already coming from__set()
which is the case when setting values as properties, which is the most common scenario. As I mentioned before, it will cause the tests to fail. I'm happy to write some quick anti-recursion logic to avoid the unlikely recursive scenario, but we can't just treat fields as properties from the dbfield because it won't always call__set()
and will result in failed tests.Please provide a clear set of instructions for triggering the problematic behaviour, so that I can write a test against it and update the code to pass that test.
It's not just about performance. People creating their own custom
setMyField()
setter methods won't be expecting those to get hit multiple times each time a field is set. I've got it down to a minimum of two times, which is already one two many but I think it's unavoidable having that second call while preserving otherwise expected behaviour. We should avoid repeated calls as it may cause unexpected side-effects in custom code which doesn't expect to be called multiple times per field set. "all logic that should be called" is being called in this PR. We're getting theDBField
instance which has the raw value and then callingsaveInto()
on that field, and then going through all of the__set()
logic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll gladly add additional recursion protection if you want me to, as I've stated. I'll also gladly add tests against the scenario you've triggered 2 different ways if you tell me what those 2 different ways are, and then update the code to make those tests pass.
I won't change this PR to call
$obj->$field = $val
because I categorically think that's incorrect - and have validated this by trying it and seeing tests fail when I try it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole __set() / setField() / setFieldValue() / saveInto() thing is so convoluted :-/
I see your point about having to call __set(), because we need to bypass the recursion protection that the arrow comes with when we go
$obj->MyField = new DBField()
Is this actually a real-world thing? Having to support the
$val instance of DBField
inDataObject::setField()
which then calls$val->saveInto($this)
is basically the root cause of the complexity here.I did a search on installer for the following regexs
->[A-Za-z]+ = new [A-Za-z]+Field
->[A-Za-z]+ = [A-Za-z]+Field::create\(
In every instances it's just assignments to an arbitary class property, not an assignment to a property on a dataobject that would trigger ViewableData::__set()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were still pre-beta I'd say lets just get rid of that and see what breaks.... but I think it's too late to remove that.
At best we could mark passing
DBField
in as property as a deprecated behaviour so it'll be easy to remove in 6, but I think it's risky to remove it now. It feels like the sort of thing that there's some magic functionality somewhere we're unaware of with bad test coverage that will break and we won't realise until someone complains.That said, if you wanna get Max's approval to remove it (we'd need that since it'd be a breaking change post-beta) I'm not super opposed to the idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need an extra set of eyes across this to sanity check it? Or do you think it's okay now that you've understood where I was coming from by calling
__set()
? Or.... ??? Not sure what the next course of action is here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had look into the feasibility of removing
$val->saveInto($this)
withinDataObject::setField()
isn't viable. The reason comes down to DBComposite fields where the following notation is supported, for instance for DBMoney$dataObject->MyDBField->methodOnDBField()
$dataObject would have have
private static $db = [ 'MyDBField' => MyDBComposite::class ];
(I think)