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

[Bug]: complexCamelCaseModels will break the routes #4

Closed
niccolofavari opened this issue Nov 10, 2023 · 8 comments · Fixed by #9
Closed

[Bug]: complexCamelCaseModels will break the routes #4

niccolofavari opened this issue Nov 10, 2023 · 8 comments · Fixed by #9

Comments

@niccolofavari
Copy link

What happened?

I see a 404 error when nesting resources with LongCamelCaseNames.

How to reproduce the bug

For example I have a model named ProjectSpNature (with the resource named ProjectSpNatureResource) and the generated route as per artisan route:list is

admin/projects/{projectRecord?}/project-sp-natures/{project sp natureRecord?}/building-blocks/create

instead of

admin/projects/{projectRecord?}/project-sp-natures/{projectSpNatureRecord?}/building-blocks/create

This creates a 404 error probably because of the spaces inside the parameter

Package Version

1.0.0-alpha.5

PHP Version

8.2.12

Laravel Version

10.30.1

Which operating systems does with happen with?

macOS

Notes

I think the solution could be changing the helper function that gets the route parameter

from

if (! function_exists('Guava\Filament\NestedResources\get_model_route_parameter')) {
    function get_model_route_parameter(string | Model $model): string
    {
        return get_model_label(is_string($model) ? $model : $model::class) . 'Record';
    }
}

to

if (! function_exists('Guava\Filament\NestedResources\get_model_route_parameter')) {
    function get_model_route_parameter(string | Model $model): string
    {
        return Str::camel(get_model_label(is_string($model) ? $model : $model::class)) . 'Record';
    }
}
@niccolofavari niccolofavari added the bug Something isn't working label Nov 10, 2023
@lukas-frey
Copy link
Contributor

Hi, thanks a lot for the bug report! I'll try to work on a fix as soon as I can. You're also welcome to make a PR if you want.

@acepoblete
Copy link
Contributor

@lukas-frey nice work on the plugin, it is super helpful. Had a question about the route names. Looks like the parameters from the route slug are not being removed. Using the example from this issue, they appear like this

admin.projects.{projectRecord?}.project-sp-natures.{project sp natureRecord?}.building-blocks.create

when normally I believe they are like this

admin.projects.project-sp-natures.building-blocks.create

Just wondering if this was by design or if it might change in future releases.

@lukas-frey
Copy link
Contributor

@acepoblete Great catch! It's not by design, I didn't really know it did that until now. Are you actually interacting directly with these slugs in your app?

I will fix it in the next release.

@tobias-grasse
Copy link

I can confirm @niccolofavari 's report. My parent resource is named JobCampaign, which results in the route admin/job-campaigns/{job campaignRecord?}/child-resource/{record} and produces a 404.

I believe the root cause is using Filament's get_model_label helper in this package's get_model_route_parameter helper, which produces a space-separated version of the model class name. I assume that the Filament helper get_model_label is intended for UI/display labels, and not for route parameters. I'll look into possible fixes as well :-)

@acepoblete
Copy link
Contributor

acepoblete commented Nov 16, 2023

@acepoblete Great catch! It's not by design, I didn't really know it did that until now. Are you actually interacting directly with these slugs in your app?

I will fix it in the next release.

I have a custom tab component for navigating between child resources. It's not very smart, so I have to manually generate the URLs for each tab. I noticed that route parameters when trying to figure out what route names to use for the child resources. I'll have a PR for this fix shortly.

moved this to issue #5

Copy link

🎉 This issue has been resolved in version 1.0.0-alpha.6 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Copy link

🎉 This issue has been resolved in version 1.0.0-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Copy link

🎉 This issue has been resolved in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants