API Remove custom logic in favour of Form::saveInto() #1269
Conversation
GuySartorelli
force-pushed
the
pulls/6/form-saveinto-element
branch
2 times, most recently
from
9ec3203
to
13c1efe
Compare
| * Arrive here from FormRequestHandler::httpSubmission() during a POST request to | ||
| * /admin/linkfield/linkForm/<LinkID> | ||
| * Arrive here from FormRequestHandler::httpSubmission() during a POST request. |
There was a problem hiding this comment.
Unrelated change here to remove copy/paste error that references linkfield
GuySartorelli
force-pushed
the
pulls/6/form-saveinto-element
branch
from
13c1efe
to
57e2b67
Compare
| /** | ||
| * Remove the pseudo namespaces that were added in namespaceFields() | ||
| */ | ||
| public function removeNamespaceFromFields(FieldList $fields, array $context): void |
There was a problem hiding this comment.
Using the same method signature as the corresponding namespaceFields() since they're essentially the reverse of one another.
| /** | ||
| * Remove the pseudo namespaces that were added in namespaceFields() | ||
| */ | ||
| public function removeNamespaceFromFields(FieldList $fields, array $context): void |
There was a problem hiding this comment.
There's no test for namespaceFields() nor for the old removeNamespacesFromFields() method on ElementalAreaController... these methods are both indirectly tested already.
But to avoid ping pong, I'll add tests for both of these methods after rebasing on top of the CMS 5 PR.
| $dataWithoutNamespaces = static::removeNamespacesFromFields($data, $element->ID); | ||
| // Remove the namespace prefixes | ||
| $factory = Injector::inst()->get(EditFormFactory::class); | ||
| $factory->removeNamespaceFromFields($form->Fields(), ['Record' => $element]); |
There was a problem hiding this comment.
The way these methods are being called on a factory class (whose purpose to a to create objects) looks weird as they're essentially utility methods. Change them to be statics and don't instantiate the factory, or just move them both to a standalone utility class as statics
There was a problem hiding this comment.
I don't want to do that, because the factory is injected and the existing namespaceFields() method could be altered in a project-specific customisation. Breaking that customisation option for the sake of "it looks weird" doesn't sit well with me.
The namespacing is intrinsic to the building of this specific form, so it makes sense for the factory to own that process.
There was a problem hiding this comment.
Looking at this again, renaming the fields on an existing form twice in order to save it seems kind of wrong
Wouldn't it make more sense to just get the factory to create an entirely new form without the namespaces, load the submitted $data into that and then call $newFormWithoutNamespaces->saveInto($element)
That would allow us to keep keep namespaceFields() as a protected method and we wouldn't even need removeNamespacesFromFields()
Update: ignore this, just occured to me the the submitted data will include namespaces
|
None of the things you've pointed out in this review block the CMS 5 PRs. Please merge the CMS 5 PRs. |
| $dataWithoutNamespaces = static::removeNamespacesFromFields($data, $element->ID); | ||
| // Remove the namespace prefixes | ||
| $factory = Injector::inst()->get(EditFormFactory::class); | ||
| $factory->removeNamespaceFromFields($form->Fields(), ['Record' => $element]); |
There was a problem hiding this comment.
Looking at this again, renaming the fields on an existing form twice in order to save it seems kind of wrong
Wouldn't it make more sense to just get the factory to create an entirely new form without the namespaces, load the submitted $data into that and then call $newFormWithoutNamespaces->saveInto($element)
That would allow us to keep keep namespaceFields() as a protected method and we wouldn't even need removeNamespacesFromFields()
Update: ignore this, just occured to me the the submitted data will include namespaces
GuySartorelli
force-pushed
the
pulls/6/form-saveinto-element
branch
from
57e2b67
to
8b826d7
Compare
GuySartorelli
force-pushed
the
pulls/6/form-saveinto-element
branch
from
8b826d7
to
a3fc862
Compare
|
|
||
| class EditFormFactoryTest extends SapphireTest | ||
| { | ||
| protected static $fixture_file = 'EditFormFactoryTest.yml'; | ||
|
|
||
| public function testFormFieldsHaveNamespaces() | ||
| { | ||
| /** @var ElementContent $record */ |
There was a problem hiding this comment.
Unrelated, but that's not needed anymore due to the addition of generics
| $factory = new EditFormFactory(); | ||
| $fields = new FieldList([ | ||
| new TextField('FieldOne'), | ||
| new LiteralField('IgnoredField', ''), |
There was a problem hiding this comment.
LiteralField is ignored because it's not a "data field" so it doesn't affect form submission
| $factory = new EditFormFactory(); | ||
| $fields = new FieldList([ | ||
| new TextField('PageElements_' . $record->ID . '_FieldOne'), | ||
| new LiteralField('IgnoredField', ''), |
There was a problem hiding this comment.
This is how the literal field will look when passed in, so we're just including it here to validate it doesn't get mangled
I don't think this warrants a note in the changelog beyond what will be automatically generated for the removed API - though feel free to disagree and if so I'll add a docs PR.
Issue
Form::saveInto()#1268