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.

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

NEW: Support dot syntax in form field names #9192

Merged
merged 8 commits into from
May 20, 2021
Merged

NEW: Support dot syntax in form field names #9192

merged 8 commits into from
May 20, 2021

Conversation

sminnee
Copy link
Member

@sminnee sminnee commented Aug 24, 2019

This change adds support for these in a few places.

  • Form::saveInto($record)
  • Form::loadDataForm($record)
  • Form::loadDataForm($_POST)

Fixes #9163

To do

  • Documentation
  • Tests

sminnee pushed a commit to sminnee/silverstripe-gridfieldextensions that referenced this pull request Aug 26, 2019
This is a companion to silverstripe/silverstripe-framework#9192 to provide the same functionality for inline
editing in GridFields

A valuable use of this is editing fields in the join-object of
a many-many-through relation.
@sminnee sminnee changed the title NEW: Support dot syntax in form field names [WIP] NEW: Support dot syntax in form field names Sep 2, 2019
@sminnee
Copy link
Member Author

sminnee commented Sep 2, 2019

Making as WIP as the tests need to be fixed, new tests need to be written, and docs

@emteknetnz
Copy link
Member

This pull request hasn't had any activity for a while. Are you going to be doing further work on it, or would you prefer to close it now?

@sminnee
Copy link
Member Author

sminnee commented Sep 8, 2020

I'm using this code in production via composer-patches so I'm definitely keen to see it make its way into mainline. I'll endeavour to get tests added and tests passing to make it mergeworthy in the next few weeks.

@chillu
Copy link
Member

chillu commented Mar 30, 2021

@sminnee Still keen on adding tests here?

@chillu
Copy link
Member

chillu commented May 20, 2021

I've put this through its paces, wrote some unit tests, and docs with extensive code examples that I've tested manually as well. Found a few functional gaps, e.g. the ability to get the field via fieldByName(). Note that the API overlaps between two use cases now: Using dots for nested CompositeField (e.g. TabSet), and using dots for relationship saving. One natural limitation is that you can't do both:

new FieldSet([
  new TextField('My.Field'),
  new TabSet('My', [
      new TextField('Field'),
  ])
]);

I think that's acceptable, and very unlikely to happen in the real world.

I've also considered the security aspects of this, and can't fault it. You'll need to opt-in to this behaviour by whitelisting the relationships which are allowed to be written. There's some sensitivity about explicitly checking can*() permissions on the nested relationships, and being careful about create vs. update (since DataObject->getComponent() will auto-create a non-existent relationship). This is clearly flagged in the tutorial.

@emteknetnz @dhensby @sminnee Good to merge?

Marginally related gotcha: Some unrelated FormTest failures caused by #9948.

@chillu chillu changed the title [WIP] NEW: Support dot syntax in form field names NEW: Support dot syntax in form field names May 20, 2021
Sam Minnee and others added 8 commits May 20, 2021 20:32
 
This change adds support for these in a few places.

 - Form::saveInto($record)
 - Form::loadDataForm($record)
 - Form::loadDataForm($_POST)

Fixes #9163
The functioning of dot-syntax in form fields mean that .s are more
likely to appear in names. This breaks javascript behaviour in HTML IDs 
and I believe is an invalid character for them.
This avoids having arbitrary differences between a join object and a
has-one relation.
@chillu
Copy link
Member

chillu commented May 20, 2021

Alright got a thumbs up from a core committer (Dan), and the work is semi peer reviewed, so I'm calling this done! Just added another commit with a changelog entry to raise the visibility of the tutorial.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support dot syntax in form field names
5 participants