FIX Ensure files can be removed from elemental blocks #1267
Conversation
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
GuySartorelli
commented
Nov 6, 2024
Comment on lines
+142
to
144
| // 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); |
There was a problem hiding this comment.
Really we should just rename the fields in the form, and then call $form->saveInto($element) - but doing that now would be an API break because someone may have customised the updateFromFormData() method in their custom elements.
GuySartorelli
commented
Nov 6, 2024
| continue; | ||
| } | ||
|
|
||
| $cmsFields = $this->getCMSFields()->saveableFields(); |
There was a problem hiding this comment.
Getting the saveableFields() explicitly - things like LiteralField don't need to be touched here.
GuySartorelli
commented
Nov 6, 2024
Comment on lines
+660
to
+661
| foreach ($cmsFields as $fieldName => $field) { | ||
| $datum = $data[$fieldName] ?? null; |
There was a problem hiding this comment.
If there's data, set to the value in the data - otherwise set to null.
This is still skipping a bunch of stuff that $form->saveInto() would do, but fully replicating that logic isn't in scope here. This change is the least disruptive change required to fix the bug.
2 tasks
GuySartorelli
force-pushed
the
pulls/5.3/allow-removing-files
branch
from
94cd28b to
87a4e42
Compare
emteknetnz
requested changes
Nov 6, 2024
GuySartorelli
force-pushed
the
pulls/5.3/allow-removing-files
branch
from
87a4e42 to
527b221
Compare
emteknetnz
approved these changes
Nov 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In CMS 5.2 and earlier, all fields in the form are included in the POST data, including empty upload fields.
In CMS 5.3 the formschema form submission logic is used instead, which omits data for empty upload fields (and possibly for other empty fields as well).
Normally this would be handled just fine with
$form->saveInto($record), but elemental doesn't use that so missing data just gets ignored instead of being set to null.CI note
The new behat test relies on a test extension added in silverstripe/silverstripe-frameworktest#210
That PR needs to be merged before this can go green.
Issue