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

Fix fetching routes when there's an underscore in the field name #2905

Closed
wants to merge 4 commits into from

Conversation

pxpm
Copy link
Contributor

@pxpm pxpm commented Jun 2, 2020

The issue was raised in #2874

We were using Str::kebab to convert the function names to url-segments but that is not the best parser for this scenario because of function names.

Per convention you must define the fetch routes as public function fetchWhatever() , fetchWhatever_dontcare, fetchWhateverDontcare; anyway, beeing it a valid function name it should be a valid url segment to use. No need to parse Str to other format than Str::snake, so we can get valid fetch routes and valid function names. Also it follow Laravel convention, when adding a field with name billing_address we know for sure it will get the fetchBillingAddress or fetchBilling_address.

It get's it's urls from entity, that should match model entity and fetchEntity function.

Eg:

    public function someEntity() { } 
// entity in field: someEntity
// fetch: fetchSomeEntity
// fetch url: https://backpackforlaravel.com/admin/entity/fetch/some_entity

@dividy
Copy link

dividy commented Jun 2, 2020

Nice !
Fast answer :-)

However, not working on my side unfortunately.

With a field customer_id :

[
                'label' => 'Client',
                'type' => "relationship",
                'name' => 'customer_id',
                'entity' => 'customer',
                'attribute' => 'full_name_with_company',
                'ajax' => true,
                'placeholder' => "Choisissez un client",
                'inline_create' => [
                    'modal_class' => 'modal-dialog modal-lg',
                ]
            ],

Result :
/site/fetch/customer_id 404 (Not Found)

The only solution I found is providing the right data_source manually :

                'data_source' => '/site/fetch/customer',

But you have to do it for every field.

@pxpm
Copy link
Contributor Author

pxpm commented Jun 3, 2020

Indeed @dividy

Yesterday it seemed ok to me to setup everything with field name, but well.. I was wrong.

Reverted the PR to use entity again, but snake case it.

Copy link
Member

@tabacitu tabacitu left a comment

Choose a reason for hiding this comment

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

Looks good to me. Is this tested & ready to merge @pxpm & @dividy ?

@tabacitu
Copy link
Member

@pxpm should I merge this? Are you sure this a non-breaking change?

@pxpm
Copy link
Contributor Author

pxpm commented Jun 16, 2020

I think as of now this will be a breaking change because everyone that setup fetch might used the data_source config to avoid this error. So if we merge this, the people who did the trick to solve the problem will have their app's broken.

I am now wondering if we need it at all (snake, or camel or kebab or whatever).

The fetch "route name" comes from a function fetchSomething. That function to have a valid name would make sure we have a valid url doesn't it? Or you see a way that a function name could not be a valid url ?

So why not https://backpackforlaravel.com/admin/entity/fetch/someEntity

Who cares about url as long as it is valid ?

@tabacitu tabacitu changed the title [4.1] Fix fetching routes [4.1] Fix fetching routes when there's an underscore in the field name Jul 7, 2020
@promatik
Copy link
Contributor

Note for future us;
When this PR get's merged on v4.2, we should remove the note introduced with Laravel-Backpack/docs#244.

@tabacitu tabacitu changed the title [4.1] Fix fetching routes when there's an underscore in the field name [4.2] Fix fetching routes when there's an underscore in the field name Apr 19, 2021
@tabacitu
Copy link
Member

tabacitu commented Sep 6, 2021

So why not https://backpackforlaravel.com/admin/entity/fetch/someEntity
Who cares about url as long as it is valid ?

I don't 😀

What do you think now, 1 year later, @pxpm - is this worth doing in 4.2? I'd rather keep the breaking changes to a minimum.

@tabacitu tabacitu changed the title [4.2] Fix fetching routes when there's an underscore in the field name Fix fetching routes when there's an underscore in the field name Dec 8, 2021
@tabacitu
Copy link
Member

tabacitu commented Dec 8, 2021

I still agree to doing this, but... we have enough breaking changes in 4.2 already... Since there's a reasonable and documented workaround for people who encounter this... I'm going to postpone merging this until the next major version... 😔

I've fixed the merge conflicts, though. So that it's easier for future-us to merge it.
Sorry guys.

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@tabacitu
Copy link
Member

tabacitu commented Dec 8, 2021

Also, let's close this in favour of #3930 which does the same thing but a bit cleaner.

@tabacitu tabacitu closed this Dec 8, 2021
@tabacitu tabacitu deleted the fix-fetch-route branch December 8, 2021 05:36
tabacitu added a commit that referenced this pull request Dec 8, 2021
# 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.

5 participants