Skip to content

Conversation

spaceemotion
Copy link
Contributor

@spaceemotion spaceemotion commented Aug 14, 2020

I was unsure which approach to take, but I ended up with reading out the actual value from the UrlGenerator class itself, rather than trying to analyze the RouteServiceProvider. Seemed like the most stable solution to me.

Fixes #321

@jasonmccreary
Copy link
Collaborator

Per the issue comments, let's make this configurable, not simply based off the RouteServiceProvider.

@spaceemotion
Copy link
Contributor Author

@jasonmccreary changed it to use config() instead 👍

@jasonmccreary
Copy link
Collaborator

Much cleaner. 👍

I'll get this merged once the tests pass and I make some tiny text tweaks. However, I may wait to tag until next week. You can use dev-master if you want it right away.

@@ -0,0 +1,5 @@


Route::resource('foo', App\Http\Controllers\FooController::class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately, this is not (currently) allowed unless the RouteServiceProvider::$namespace property is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There could be a note about this in the config file.

@jasonmccreary jasonmccreary self-requested a review August 14, 2020 21:26
Copy link
Collaborator

@jasonmccreary jasonmccreary left a comment

Choose a reason for hiding this comment

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

Unfortunately the FQCN is only allowed with the tuple syntax. In order to use an FQCN for a single route (or resource) you must unset the namespace property within your RouteServiceProvider.

So, while I still like the config, in order to fully achieve what you're after, there would also have to be some additional logic to check that property again. Which I'm on the fence about...

@spaceemotion
Copy link
Contributor Author

Since this is a configurable value, my guess is that this should be a deliberate action by the user. If they enable it without knowing what it does...

@jasonmccreary
Copy link
Collaborator

Hmmm... I considered a PR to the framework to allow the FQCN even with this RouteServiceProvider property set. But according to Taylor, it seems as though this property is no longer set by default starting with Laravel 8.

As such, I am comfortable with it as is, and to your point, guiding users to unset the property and/or upgrade to the latest version.

However, we may wish to change the option name to generate_fqcn_routes or something...

@spaceemotion
Copy link
Contributor Author

spaceemotion commented Aug 14, 2020

Hm, if Laravel 8 will leave the value blank, the config value shouldn't just be false by default then, right? Probably should check Application::VERSION if it starts with 7 and disable it.

jasonmccreary and others added 2 commits August 14, 2020 17:58
Co-authored-by: spaceemotion <spaceemotion@users.noreply.github.com>
Co-authored-by: spaceemotion <spaceemotion@users.noreply.github.com>
@jasonmccreary jasonmccreary merged commit 7c9a32e into laravel-shift:master Aug 14, 2020
@spaceemotion spaceemotion deleted the 321-generate-route-tuples branch August 15, 2020 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to generate routes using class/method tuple
2 participants