FIX Ensure getters and setters are respected

[GuySartorelli]

Instances of DBField stopped using the __set() logic in #10614 when saveInto() was called on them. This caused the regression in the issue.

This PR explicitly uses the __set() logic, and includes tests to ensure it works as expected and we're not doing anything recursively.

Parent issue

• CMS 5 beta regression: setter method not called when saving field through edit form #10707

[emteknetnz]

Suggested change

	if ($this->value instanceof self) {
	$this->value->saveInto($dataObject);
	} else {
	$dataObject->__set($fieldName, $this->value);
	}
	$dataObject->$fieldName = $this->value;

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

[GuySartorelli]

We don't need $this->value->saveInto($dataObject) as it will be called here if $val is a DBField.

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 another DBField instance as its value, which has another DBField instance as its value, etc. What you're proposing will go through the saveInto() => __set() => setField() => saveInto() loop for every instance. The way I've got it, all of that is resolved in saveInto() 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 new DBField we're setting as the value has a series of nested DBField 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.

[GuySartorelli]

We should be using the arrow notation that calls __set() rather than calling __set() directly

What do you mean "the arrow notation"? Do you mean just pretend we're setting a property? If we do that, the tests fail.

because there's some infinite recursion protection only when using arrow notation which I think happens at the PHP level

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 in saveInto which wouldn't be difficult to do.

because calling __set() directly just looks weird

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.

[emteknetnz]

What do you mean "the arrow notation"? Do you mean just pretend we're setting a property?

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.

which is extremely unlikely

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.

This shortcuts passed that, avoiding repeated calls through this chain when it's not needed

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.

[GuySartorelli]

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.

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.

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.

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.

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.

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 the DBField instance which has the raw value and then calling saveInto() on that field, and then going through all of the __set() logic.

[GuySartorelli]

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.

[emteknetnz]

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()

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.

Is this actually a real-world thing? Having to support the $val instance of DBField in DataObject::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()

[GuySartorelli]

Is this actually a real-world thing? Having to support the $val instance of DBField in DataObject::setField() is basically the root cause of the complexity here.

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.

[GuySartorelli]

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?

[emteknetnz]

I've had look into the feasibility of removing $val->saveInto($this) within DataObject::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)

[kinglozzer]

Maybe we need an extra set of eyes across this to sanity check it

Would it be safer to revert the changes to ViewableData::setField()/getField()/hasField() and instead add the annotation #[AllowDynamicProperties] to ViewableData and preserve the 4.x behaviour?

[michalkleiner]

Maybe we need an extra set of eyes across this to sanity check it

Would it be safer to revert the changes to ViewableData::setField()/getField()/hasField() and instead add the annotation #[AllowDynamicProperties] to ViewableData and preserve the 4.x behaviour?

Sometimes we are so consumed with all the detail we don't take a step back to reevaluate. Good thinking, @kinglozzer!

[GuySartorelli]

That would technically be safer, but it's also kicking the can down the road. Dynamic properties are a deprecated behaviour, so it'll be removed in a future version of PHP. If we can just resolve it here without reverting the changes we've made I'd prefer that - or else we're liable to hit all of the same problems and debugging etc again when they do remove that feature.

[emteknetnz]

Responding to the comments above, I also don't want to rollback to dynamic properties

This approach is probably as good as we're going to get things for now, we still need to support DBComposite which allows up to go $dataObject->DBField->methodOnDBField() so removing $val->saveInto($dataObject) in DataObject::setField() isn't viable at this stage, which is a shame because it would allow us to greatly simplify things

@GuySartorelli I've run this PR again both installer and sink, I've also re-run installer 5.0 and sink 5.0 - looks like there may be an issue with an admin unit-test with this PR

PR runs

• https://github.com/emteknetnz/silverstripe-installer/actions/runs/4297848040/jobs/7491323096
• https://github.com/emteknetnz/recipe-kitchen-sink/actions/runs/4297848351/jobs/7491323401

5.0 runs (triggered recently, both green)

• https://github.com/silverstripe/silverstripe-installer/actions/runs/4298114935
• https://github.com/silverstripe/recipe-kitchen-sink/actions/runs/4298115877

[GuySartorelli]

Those failures are unrelated to this PR and are caused by not having rebased this PR before running the tests. Confirmed this by running locally (failed), then rebasing and running again locally (passed)
Rebased now. They'll go green if you try it again.

[emteknetnz]

Yup green - updated runs

• https://github.com/emteknetnz/silverstripe-installer/actions/runs/4299228436
• https://github.com/emteknetnz/recipe-kitchen-sink/actions/runs/4299228949

[kinglozzer]

That would technically be safer, but it's also kicking the can down the road. Dynamic properties are a deprecated behaviour, so it'll be removed in a future version of PHP.

Fair enough, the RFC isn’t entirely clear about this - it’s called “Deprecate dynamic properties” but also seems to imply that there’s no real plan for when they might be removed yet:

We may still wish to remove dynamic properties entirely at some later point. Having the #[AllowDynamicProperties] attribute will make it much easier to evaluate such a move, as it will be easier to analyze how much and in what way dynamic properties are used in the ecosystem.

My other suggestion was going to be to investigate the feasibility of using ViewableData extends \stdClass but it looks like this has been resolved 🙂