Skip to content

FIX Ensure value is in the correct format after save#386

Merged
GuySartorelli merged 1 commit intosilverstripe:4.2from
creative-commoners:pulls/4.2/value-string-array
Jun 12, 2025
Merged

FIX Ensure value is in the correct format after save#386
GuySartorelli merged 1 commit intosilverstripe:4.2from
creative-commoners:pulls/4.2/value-string-array

Conversation

@emteknetnz
Copy link
Copy Markdown
Member

Issue #385

@emteknetnz emteknetnz force-pushed the pulls/4.2/value-string-array branch 4 times, most recently from 5557390 to 418a05b Compare June 5, 2025 06:02
@emteknetnz emteknetnz marked this pull request as ready for review June 5, 2025 23:01
Copy link
Copy Markdown
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

The reason SiteConfig does this and other admin sections don't is that SiteConfigLeftAndMain (and CMSProfileController which presumably would have the same problem) don't use the response negotiator.

i.e the problem goes away if we make this change in SiteConfigLeftAndMain::save_siteconfig():

+ $this->setResponse($this->getResponseNegotiator()->respond($this->getRequest()));
  $this->response->addHeader(
      'X-Status',
      rawurlencode(_t('SilverStripe\\Admin\\LeftAndMain.SAVEDUP', 'Saved.'))
  );
- return $form->forTemplate();
+ return $this->getResponse();

However, we then also need to update the validationResponseCallback defined in getEditForm. And that's still not the root cause - there's no sensible reason why rendering the form should result in adding extra brackets around the value. It speaks to a problem in the PHP code of the form field itself.

The root cause

The flow for most admin sections uses getResponseNegotiator() and renders the form fresh using getEditForm() rather than just rendering the form that came with the submission.

Calling getEditForm() fresh results in calling $form->loadDataFrom($record); which goes through MultiLinkField::setValue($value, $record) which loads the raw IDs from the relation, which do not have the '[]' brackets around the ID array.

The flow for SiteConfigLeftAndMain skips loading data from the record before rendering the form, which means it has the brackets around the value, because those brackets aren't filtered out when setSubmittedValue() is called on the field.

The fix

Add this method to MultiLinkField:

public function setSubmittedValue($value, $data = null)
{
    $value = rtrim(ltrim($value, '['), ']');
    return $this->setValue($value, $data);
}

No need for additional javascript logic.

@emteknetnz emteknetnz force-pushed the pulls/4.2/value-string-array branch from 418a05b to 6b76021 Compare June 11, 2025 23:23
Comment thread src/Form/MultiLinkField.php Outdated
@emteknetnz
Copy link
Copy Markdown
Member Author

Have updated to do it all in PHP. There was an existing method convertValueToArray() with unit tests to handle converting values so I just expanded on it

@emteknetnz emteknetnz force-pushed the pulls/4.2/value-string-array branch from 6b76021 to 381a99a Compare June 12, 2025 01:37
@GuySartorelli
Copy link
Copy Markdown
Member

#386 (comment) to be responded to

@emteknetnz emteknetnz force-pushed the pulls/4.2/value-string-array branch from 381a99a to f2b7fdc Compare June 12, 2025 23:11
Copy link
Copy Markdown
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM

@GuySartorelli GuySartorelli merged commit 1de6548 into silverstripe:4.2 Jun 12, 2025
15 checks passed
@GuySartorelli GuySartorelli deleted the pulls/4.2/value-string-array branch June 12, 2025 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants