Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Bugfix values function #3192

Merged
merged 5 commits into from
Sep 15, 2020

Conversation

mahasadhu
Copy link
Contributor

@mahasadhu mahasadhu commented Sep 9, 2020

Values function in fluent API not calling the inner / callback / anonymous function sent to it as parameter

Copy link
Contributor

@pxpm pxpm left a comment

Choose a reason for hiding this comment

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

Apart from my comment I very much welcome this PR. Thank you @mahasadhu

This must have slipped while we refactored Filters into classes. So sorry for the trouble.

If you don't have time to make the suggested changes let us know so we can finish this up and merge the PR.

Wish you the best,
Pedro

@@ -285,7 +285,7 @@ public function label($value)
*/
public function values($value)
{
$this->values = $value;
$this->values = is_callable($value) ? $value() : $value;
Copy link
Contributor

Choose a reason for hiding this comment

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

We had trouble using this kind of check in the past, who knows what values could endup there 😃

Just to make sure we don't get into the same hole I would go here with:

$this->values = (!is_string($value) && is_callable($value)) ? $value() : $value;

If the string is a php core function we know what is going to happen 👍

@mahasadhu
Copy link
Contributor Author

@pxpm Hi Pedro, thanks for checking 😃

Sorry I did not know that. I've pushed some updates to the code.

Another idea, we can just close this PR and update the docs from this:
https://backpackforlaravel.com/docs/4.1/crud-fluent-syntax
image

to, for example, this:

            CRUD::filter('department')
                ->type('select2')
                ->label(trans('backpack::base.job_category'))
                ->values(\App\Models\Department::all()->pluck('name', 'id')->toArray())
                ->whenActive(function($value) { // if the filter is active
                    $this->crud->query->whereHas('project.department', function ($query) use ($value) {
                        $query->where('id', $value);
                    });
                });

Because (CMIIW) it seems no one has realized this errors yet. But it's up to you though, just a thought.

Thank you!

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@pxpm
Copy link
Contributor

pxpm commented Sep 10, 2020

Hello @mahasadhu thank you very much once again 👍

The refactor to use Fluent api for Filters happened not so long time, so that's why I think not so many people switch from old array definition to the new fluent syntax.

The array definition supports anonymous functions in value key, this was just a mistake we let slip when refactoring to the Fluent API.

With your PR merged the documentation should be ok ?

Best,
Pedro

@mahasadhu
Copy link
Contributor Author

mahasadhu commented Sep 10, 2020

No problem at all @pxpm

Ah I see...
Yes it should be ok, no need to update the docs

Thank you

@tabacitu
Copy link
Member

Thank a lot @mahasadhu & @pxpm . You're right, of course.
Merged, will be tagged later today, and be available as a patch update with composer update.

Keep 'em coming! Cheers.

@tabacitu tabacitu merged commit 6fb2d41 into Laravel-Backpack:master Sep 15, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants