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

(Automatically) Convert empty association properties for SAVE_REPLACE compatibility? #9474

Closed
2 of 3 tasks
ndm2 opened this issue Sep 17, 2016 · 20 comments
Closed
2 of 3 tasks

Comments

@ndm2
Copy link
Contributor

ndm2 commented Sep 17, 2016

This is a (multiple allowed):

  • bug
  • enhancement
  • feature-discussion (RFC)

Let's say we have a form that supports adding/removing hasMany/belongsToMany associated records that are using the SAVE_REPLACE save strategy.

$this->Form->input('parent_field');
$this->Form->input('association.0.field');
$this->Form->input('association.1.field');
$this->Form->input('association.2.field');
// ...

When removing for example association.2.field, the saving process would delete the record accordingly. When however removing all of them, so that we'd be left with only

$this->Form->input('parent_field');

then the association property wouldn't be affected anymore at all, and nothing would change, ie the records wouldn't be deleted.

To workaround that one have to do something like adding a hidden field with the name of the association property, and check that before marshalling and replace it with an empty array accordingly, like for example

$this->Form->input('parent_field');
$this->Form->hidden('association', ['value' => '']);
$this->Form->input('association.0.field');
$this->Form->input('association.1.field');
$this->Form->input('association.2.field');
// ...
public function beforeMarshal(Event $event, \ArrayObject $data, \ArrayObject $options)
{
    if (isset($data['association']) && $data['association'] === '') {
        $data['association'] = [];
    }
}

That way all records would be deleted properly when none of the association.*.field inputs would be submitted.

I'm wondering if this is something that may fit in the core? Maybe as an optional behavior that can be enabled via the associated option?

@markstory markstory added the ORM label Sep 17, 2016
@markstory markstory added this to the 3.4.0 milestone Sep 17, 2016
@markstory
Copy link
Member

Making an empty string replace the association with an empty list sounds like a reasonable default behavior to me.

@lorenzo
Copy link
Member

lorenzo commented Sep 18, 2016

Could this be used in a malicious way? Or could it be a surprise for users to discover that there is a way to delete all associated records from a form?

@markstory
Copy link
Member

@lorenzo Good point. It could be used for bad purposes.

@ndm2
Copy link
Contributor Author

ndm2 commented Sep 18, 2016

I was a little worried too at first, hence the suggestion to make this optional, but the more I think about it, the less concerned I am. By default users are currently able to delete all but one record, so I'd say that one record more doesn't really make a difference.

I'd think that if developers use the replace save strategy, then they usually want exactly that to be possible. If they don't want users to be able to remove associated records via a form depending on the current context, then they would have to implement additional measures anyways, like switching to the append strategy, or better yet the other way around, enable the replace strategy only where explicitly required.

@markstory
Copy link
Member

@ndm2 It sounds like the kind of change that would have to go into 3.x.0 as to not surprise people.

@lorenzo
Copy link
Member

lorenzo commented Sep 19, 2016

You have a point in that today all records but one can be removed

@lorenzo
Copy link
Member

lorenzo commented Sep 19, 2016

If your were presented with an external API you need to use, and it required require sending an empty string instead of an array, like in every other case, what would you think of such API?

@inoas
Copy link
Contributor

inoas commented Sep 19, 2016

Can't this be fixed at the controller/table level with an option flag (e.g. declared within action code)?

@markstory
Copy link
Member

@lorenzo In an API context couldn't you push an empty list?

@ndm2
Copy link
Contributor Author

ndm2 commented Sep 19, 2016

@lorenzo I wouldn't like that, but as @markstory mentioned, an API may be be able to receive data in a different format that can properly represent empty list structures, like for example JSON. In a form data context there is no way to send an empty list, so I'd say an empty string is the next best thing.

@inoas
Copy link
Contributor

inoas commented Sep 19, 2016

... or maybe 'false'?
Edit:

$this->Form->hidden('association', ['value' => 'false']);
// e.g:
$this->Form->hidden('Roles', ['value' => 'false']);

@ndm2
Copy link
Contributor Author

ndm2 commented Sep 19, 2016

@inoas Looks a little akward as it's very different to how to usually pass empty values in a form context, and I could easily see people pass the value in an unquoted fashion by accident, but generally something like that would work too of course.

I don't think there's any perfect solution, so I'd personally tend to simply use whatever is the least error prone, most intuitive, and easiest to remember.

@inoas
Copy link
Contributor

inoas commented Sep 19, 2016

I bet ['value' => false] will render as "value = "0" (untested). If it renders like "value" => "false" then that's okay...

@markstory
Copy link
Member

['value' => false] will turn into value='' in HTML.

@lorenzo
Copy link
Member

lorenzo commented Sep 20, 2016

@markstory can you put an empty list using form-data as a content type?

@markstory
Copy link
Member

@lorenzo I don't think that is possible article[comments]= makes a string while article[comments][]= makes a list with 1 element of '' in it.

@lorenzo
Copy link
Member

lorenzo commented Sep 20, 2016

I see, then accepting the empty string makes sense

@markstory markstory modified the milestones: 3.4.0, 3.5.0 Jan 30, 2017
ndm2 pushed a commit to ndm2/cakephp that referenced this issue Apr 3, 2017
Adds supports for converting empty association properties to empty
arrays for `hasMany` and `belongsToMany` associations. This helps
with removing all associated records when saving with the `replace`
save strategy.

Refs cakephp#9474
@ndm2
Copy link
Contributor Author

ndm2 commented Apr 3, 2017

Finally a little time to get some stuff off of my todo list... I had a a look into this again, and after implementing it in the marshaller, I've noticed that BelongsToMany associations already have that behavior built in on association level 😑

So I guess similar could simply go into HasMany as well (instead of doing this in the marshaller)?

@lorenzo @markstory (just in case, not sure if anyone's still subscribed after felt 12 1/2 years)

@markstory
Copy link
Member

Making HasMany and BelongsToMany association classes work similarly sounds like a good idea to me.

ndm2 pushed a commit to ndm2/cakephp that referenced this issue Apr 4, 2017
Adds support for treating `null`, `false`, and empty strings as empty
sets of associated data, similar to how `BelongsToMany` handles such
values.

Also introduces bailing out early on empty sets for not yet persisted
entities when using the `replace` save strategy.

Solves cakephp#9474
@ndm2
Copy link
Contributor Author

ndm2 commented Apr 5, 2017

Closing as the enhancement has been merged.

@ndm2 ndm2 closed this as completed Apr 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants