Laravel-Backpack / addons

A place for the Backpack community to talk about possible Backpack add-ons.
5 stars 2 forks source link

[Editable Columns][bug] - Error on save when using guarded array instead of fillable #43

Closed gregraab closed 2 years ago

gregraab commented 2 years ago

Unable to save any field that is not specifically in the fillable array.

We currently have a large number of fields in our model and use the $guarded array instead of fillable - protected $guarded = ['id'];. When doing a save on an editable field, get an error for all.

In MinorUpdateOperation.php line 46 the code does if (!in_array($input['attribute'], $this->crud->model->getFillable())) { which appears to come back empty and so the test fails, even though the field is indeed fillable.

Changing this to if (!$this->crud->model->isFillable($input['attribute'])) { seems to fix the problem and I would guess would be slightly more performant for large fillable arrays like ours would have to be.

I have not done extensive testing and the editable repo does not seem to be accessible ( the link to the repo on the docs page 404s) so I can not do a pull request for this change.

Is there a way to override this function in our code base so we can move forward for now?

PHP VERSION:

PHP 7.4.27 (cli) (built: Dec 14 2021 17:17:06) ( NTS ) Copyright (c) The PHP Group Zend Engine v3.4.0, Copyright (c) Zend Technologies with Zend OPcache v7.4.27, Copyright (c), by Zend Technologies with Xdebug v2.9.8, Copyright (c) 2002-2020, by Derick Rethans

LARAVEL VERSION:

v8.83.23@bdc707f8b9bcad289b24cd182d98ec7480ac4491

BACKPACK VERSION:

5.3.4@ca7f97e3c644e35f134596281192dd6628ab449a

pxpm commented 2 years ago

Hello there, if it's a bug we fix it. No worries, I get what you are saying, I will ping @promatik cause he knows better the ins-outs of editable columns. It's something we didn't predict as our operation technically performs a mass assign, and only attributes in the fillable array should be mass assigned.

If @promatik got a lot on his plate please move the issue back to me and I will have a look at it tomorrow.

Cheers

gregraab commented 2 years ago

Thanks @pxpm.

My understanding is that you can either specify fillable additive via the fillable array or excluding via the guarded array. Both specify which fields are fillable as documented by Laravel.
I would actually think the Laravel call getFillable() would return all fillable fields, but it appears that it returns what is in the fillable array instead, which causes the issue.
isFillable() seems to check both and just return if the field is fillable regardless of which way it was defined.

This still may not be the best solution as I am not a Laravel expert, but it does seem to work in my testing.

Greg

pxpm commented 2 years ago

Thank you @gregraab With such a good explanation it should not take much time to get to the bottom of this issue . Very appreciated šŸ‘

Cheers

promatik commented 2 years ago

Hey @gregraab!

You are TOTALLY right šŸ‘Œ I have suggested your fix in a new PR, I think it will soon be merged, we'll let you know.

Sorry for taking so long, and thank you for providing the fix šŸ™Œ

gregraab commented 2 years ago

Thanks for the update @promatik. Is there a maintenance or bug release coming out in the near future that this may be part of? If so, an approximate time frame we could expect that? We are wanting to get this feature rolled out to production soon if possible.

tabacitu commented 2 years ago

Merged & tagged. A composer update backpack/editable-columns should get you the fix @gregraab .

Thanks a lot for the detailed bug report šŸ™

PS. Sorry it took so much time to actually see this, it slipped through the cracks. We're working on a system so that can't happen any more - we're already down from 100 issues to less than 25 in our main repo šŸ’Ŗ

gregraab commented 2 years ago

Hey @tabacitu. Doesn't look like it made it into the current branch?

Ran composer update, which did pull down a new version of editable columns, but code change isn't there. Issue/Fix is not listed in the patch notes for the recent branch either.

Lock file operations: 0 installs, 1 update, 0 removals
  - Upgrading backpack/editable-columns (2.0.7 => 2.0.8)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 0 installs, 1 update, 0 removals
  - Downloading backpack/editable-columns (2.0.8)
  - Upgrading backpack/editable-columns (2.0.7 => 2.0.8): Extracting archive

Thanks

pxpm commented 2 years ago

Hello @gregraab it should be on 2.0.9 released 12 hours ago

image

There must be something broken in the release cycle. Sorry for that šŸ™ I already notified @tabacitu about it.

He will comeback with news!

tabacitu commented 2 years ago

Fixed!

2.0.9 is live now https://backpackforlaravel.com/products/editable-columns/CHANGELOG.md

Thanks again @gregraab