Skip to content

Make LocationModel compatible with $guarded fields#53

Merged
LukeTowers merged 2 commits intorainlab:masterfrom
voveson:patch-1
Jun 26, 2018
Merged

Make LocationModel compatible with $guarded fields#53
LukeTowers merged 2 commits intorainlab:masterfrom
voveson:patch-1

Conversation

@voveson
Copy link
Copy Markdown
Contributor

@voveson voveson commented Jun 12, 2018

Closes #52.

Checks if the implementing model has defined $fillable properties before adding location properties as fillable. This allows the developer to choose either the whitelist approach (eg. defining $fillable fields), or the blacklist approach (eg. defining $guarded fields) for mass-assignment protection.

Closes rainlab#52.

Checks if the implementing model has defined `$fillable` properties before adding location properties as fillable. This allows the developer to choose either the whitelist approach (eg. defining `$fillable` fields), or the blacklist approach (eg. defining `$guarded` fields) for mass-assignment protection.
@LukeTowers
Copy link
Copy Markdown
Contributor

Shouldn't this check the guarded properties instead? A model could define neither but disallow mass assignment until one of them is defined, in which case we need the behavior to add those fillable properties.

@voveson
Copy link
Copy Markdown
Contributor Author

voveson commented Jun 13, 2018

Ah, you're right. I didn't realize that the default behavior is to guard everything. It looks like the GuardsAttributes trait defines guarded as follows:

protected $guarded = ['*'];

So you are right, if the developer does not define either $guarded or $fillable, by default everything is guarded. I will update to account for that.

If `$guarded` has not been overridden by the implementing model, then location-specific fillable fields must be added.
@LukeTowers
Copy link
Copy Markdown
Contributor

Doesn't this cause an issue if the developer has overridden the guarded property to manually blacklist guarded properties?

@voveson
Copy link
Copy Markdown
Contributor Author

voveson commented Jun 14, 2018

I hope not, since that's the very issue this PR is attempting to solve! 😉Are you seeing something different in your testing?

In my testing, this patch works as expected in all of the following cases:

  1. Developer overrides the $guarded array
  2. Developer overrides the $fillable array
  3. Developer does not override $guarded or $fillable

The only case that is not supported is if the developer overrides both $fillable and $guarded. This seems acceptable to me, since the docs explicitly state:

Of course, you should use either $fillable or $guarded - not both

@voveson
Copy link
Copy Markdown
Contributor Author

voveson commented Jun 18, 2018

Hey @LukeTowers, I understand you're busy -- I just wanted to make sure that you're not still waiting on something from me. If any other changes are needed before this can be merged in, please let me know. Thanks!

@LukeTowers
Copy link
Copy Markdown
Contributor

@voveson thanks for the reminder :)

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.

2 participants