WIP SS4 compat, rewrite to DBField #44
Conversation
The master branch of this module (3.x release line) has been made 4.x compatible a while ago, but the test config didn't reflect that
The field is not a “composite” field, since it only has one database column. The DBComposite API has changed a bit in SS4, which causes the field to break in weird ways since it has overloaded so much of the underlying core functionality. For example, the field never gets written since it doesn’t set MyFieldValue, and DataObject->prepareManipulationTable() skips MyField because it’s not a database column. Rather than fixing these quirks, it’s much easier to rewrite the field as a DBText. TODO: - Document db column migration path (from MyFieldValue to MyField) - Change from serialise() to json_encode() for security reasons
chillu
changed the title
chillu
changed the title
Yep, this is probably my biggest desire with the SS4 move :) |
|
closes #46 |
| } | ||
| public function testSetSerialisedStringAsProperty() { | ||
| $obj = new MultiValueFieldTest_DataObject(); | ||
| $obj->MVField = serialize(['One', 'Two']); |
There was a problem hiding this comment.
Update to json_encode() if prior comments are correct.
| $this->assertTrue($field->isChanged()); | ||
| public function testSetSerialisedStringWithSetValue() { | ||
| $obj = new MultiValueFieldTest_DataObject(); | ||
| $obj->obj('MVField')->setValue(serialize(['One', 'Two'])); |
There was a problem hiding this comment.
Update to json_encode() if prior comments are correct.
|
@nyeholt can you comment on what you were going for here and how it is reflected in the updated PR? |
|
@muskie9 - rather than using serialize and unserialize in get/setValue, it should be using json_encode/json_decode. This could be done in a backwards compatible manner, by looking for "a:" as the starting characters of the field value and assuming the old format, otherwise using json_decode. |
|
I've been digging into this a bit more and am wondering if some classes such as |
|
After looking through this some more I did a rebase and pushed some work up to a fork not to muddy this PR. I have all tests passing with the exception of testing |
|
I believe Ingo's PR changes the field type that MultiValueField inherits from, so the way setValue works is quite different as it's not longer having to handle writing as a composite field would (which is a much simpler/better approach). |
|
Thanks @nyeholt, after digging into this a bit more, and comparing to some things in framework |
|
FYI - Release 5.0.0 (https://packagist.org/packages/symbiote/silverstripe-multivaluefield) covers this off now; there's a few reasons that it needs to be a composite field (due to how normal text fields don't give you a chance to modify the field value when it's loaded from the DB if it's not a composite field). This also includes switching to json_encode instead of serialise, but in a BC manner. |
This started out as an innocent "fix tests for 4.x" task: The master branch of this module (3.x release line) has been made 4.x compatible a while ago, but the test config didn't reflect that
But it turns out the field is quite broken with the SS4 ORM:
The field is not a “composite” field, since it only has one database column.
The DBComposite API has changed a bit in SS4, which causes the field to break in weird ways
since it has overloaded so much of the underlying core functionality. For example,
the field never gets written since it doesn’t set MyFieldValue, and
DataObject->prepareManipulationTable() skips MyField because it’s not a database column.
Rather than fixing these quirks, it’s much easier to rewrite the field as a DBText.
TODO:
@sanderha @wernerkrauss @digitall-it @brasileric Anybody keen to review and help get this over the line?