Skip to content
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.

Sign up for GitHub

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

Jump to bottom

API Remove custom logic in favour of Form::saveInto() #1269

Merged
merged 1 commit into from
Nov 7, 2024
+70 −55
Merged

API Remove custom logic in favour of Form::saveInto() #1269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 9 additions & 33 deletions src/Controllers/ElementalAreaController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,7 @@ public function getElementForm(): Form
}

/**
* Arrive here from FormRequestHandler::httpSubmission() during a POST request to
* /admin/linkfield/linkForm/<LinkID>
* Arrive here from FormRequestHandler::httpSubmission() during a POST request.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change here to remove copy/paste error that references linkfield

* The 'save' method is called because it is the FormAction set on the Form
*/
public function save(array $data, Form $form): HTTPResponse
Expand Down Expand Up @@ -331,13 +330,15 @@ public function save(array $data, Form $form): HTTPResponse
$this->jsonError(403);
}

// Remove the namespace prefixes that were added by EditFormFactory
$dataWithoutNamespaces = static::removeNamespacesFromFields($data, $element->ID);
// Remove the namespace prefixes
$factory = Injector::inst()->get(EditFormFactory::class);
$factory->removeNamespaceFromFields($form->Fields(), ['Record' => $element]);
Copy link
Member

@emteknetnz emteknetnz Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@GuySartorelli GuySartorelli Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@emteknetnz emteknetnz Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Update the record
$form->saveInto($element);
// Add the namespace prefixes back - this is necessary for the response to be handled correctly
$factory->namespaceFields($form->Fields(), ['Record' => $element]);

// Update and write the data object which will trigger model validation.
// Would usually be handled by $form->saveInto($element) but since the field names
// in the form have been namespaced, we need to handle it ourselves.
$element->updateFromFormData($dataWithoutNamespaces);
// Write the data object which will trigger model validation
if ($element->isChanged()) {
try {
$element->write();
Expand All @@ -354,31 +355,6 @@ public function save(array $data, Form $form): HTTPResponse
return $response;
}

/**
* Remove the pseudo namespaces that were added to form fields by the form factory
*
* @param array $data
* @param int $elementID
* @return array
* @deprecated 5.4.0 Will be removed without equivalent functionality to replace it.
*/
public static function removeNamespacesFromFields(array $data, $elementID)
{
Deprecation::noticeWithNoReplacment('5.4.0');
$output = [];
$template = sprintf(EditFormFactory::FIELD_NAMESPACE_TEMPLATE, $elementID, '');
foreach ($data as $key => $value) {
// Only look at fields that match the namespace template
if (substr($key ?? '', 0, strlen($template ?? '')) !== $template) {
continue;
}

$fieldName = substr($key ?? '', strlen($template ?? ''));
$output[$fieldName] = $value;
}
return $output;
}

private function reorderElements(BaseElement $element, int $afterElementID): void
{
if ($afterElementID < 0) {
Expand Down
23 changes: 19 additions & 4 deletions src/Forms/EditFormFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,8 @@ protected function getFormValidator(RequestHandler $controller = null, $name, $c
/**
* Given a {@link FieldList}, give all fields a unique name so they can be used in the same context as
* other elemental edit forms and the page (or other DataObject) that owns them.
*
* @param FieldList $fields
* @param array $context
*/
protected function namespaceFields(FieldList $fields, array $context)
public function namespaceFields(FieldList $fields, array $context): void
{
$elementID = $context['Record']->ID;

Expand All @@ -103,4 +100,22 @@ protected function namespaceFields(FieldList $fields, array $context)
$field->setName($namespacedName);
}
}

/**
* Remove the pseudo namespaces that were added in namespaceFields()
*/
public function removeNamespaceFromFields(FieldList $fields, array $context): void
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the same method signature as the corresponding namespaceFields() since they're essentially the reverse of one another.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a unit test

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

{
$elementID = $context['Record']->ID;
$template = sprintf(EditFormFactory::FIELD_NAMESPACE_TEMPLATE, $elementID, '');

foreach ($fields->dataFields() as $namespacedName => $field) {
// Only look at fields that match the namespace template
if (substr($namespacedName, 0, strlen($template)) !== $template) {
continue;
}
$newName = substr($namespacedName, strlen($template));
$field->setName($newName);
}
}
}
17 changes: 0 additions & 17 deletions src/Models/BaseElement.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,23 +646,6 @@ public function getRenderTemplates($suffix = '')
return $templateFlat;
}

/**
* Given form data (wit
*
* @param $data
* @deprecated 5.4.0 Will be removed without equivalent functionality to replace it.
*/
public function updateFromFormData($data)
{
Deprecation::noticeWithNoReplacment('5.4.0');
$cmsFields = $this->getCMSFields()->saveableFields();
foreach ($cmsFields as $fieldName => $field) {
$datum = $data[$fieldName] ?? null;
$field->setSubmittedValue($datum);
$field->saveInto($this);
}
}

/**
* Strip all namespaces from class namespace.
*
Expand Down
43 changes: 42 additions & 1 deletion tests/Forms/EditFormFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,18 @@

use DNADesign\Elemental\Forms\EditFormFactory;
use DNADesign\Elemental\Models\ElementContent;
use SilverStripe\AssetAdmin\Forms\UploadField;
use SilverStripe\Dev\SapphireTest;
use SilverStripe\Forms\FieldList;
use SilverStripe\Forms\LiteralField;
use SilverStripe\Forms\TextField;

class EditFormFactoryTest extends SapphireTest
{
protected static $fixture_file = 'EditFormFactoryTest.yml';

public function testFormFieldsHaveNamespaces()
{
/** @var ElementContent $record */
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but that's not needed anymore due to the addition of generics

$record = $this->objFromFixture(ElementContent::class, 'content_block');

$factory = new EditFormFactory();
Expand All @@ -21,4 +24,42 @@ public function testFormFieldsHaveNamespaces()

$this->assertNotNull($fields->dataFieldByName('PageElements_' . $record->ID . '_Title'));
}

public function testNamespaceFields(): void
{
$record = $this->objFromFixture(ElementContent::class, 'content_block');
$factory = new EditFormFactory();
$fields = new FieldList([
new TextField('FieldOne'),
new LiteralField('IgnoredField', ''),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LiteralField is ignored because it's not a "data field" so it doesn't affect form submission

new UploadField('FieldTwo'),
]);
$factory->namespaceFields($fields, ['Record' => $record]);

$expectedNames = [
'PageElements_' . $record->ID . '_FieldOne',
'IgnoredField',
'PageElements_' . $record->ID . '_FieldTwo',
];
$this->assertSame($expectedNames, $fields->column('Name'));
}

public function testRemoveNamespaceFromFields(): void
{
$record = $this->objFromFixture(ElementContent::class, 'content_block');
$factory = new EditFormFactory();
$fields = new FieldList([
new TextField('PageElements_' . $record->ID . '_FieldOne'),
new LiteralField('IgnoredField', ''),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

new UploadField('PageElements_' . $record->ID . '_FieldTwo'),
]);
$factory->removeNamespaceFromFields($fields, ['Record' => $record]);

$expectedNames = [
'FieldOne',
'IgnoredField',
'FieldTwo',
];
$this->assertSame($expectedNames, $fields->column('Name'));
}
}