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

Fix generated route name not correctly detected when using prefixes #441

Merged
merged 1 commit into from
Jan 23, 2021
Merged

Conversation

wfeller
Copy link
Contributor

@wfeller wfeller commented Jan 20, 2021

Use Str::contains instead of Str::startsWith to verify if route name is generated.

Fixes #440

Use Str::contains instead of Str::startsWith to verify if route name is generated.
@stayallive stayallive changed the title Fix #440 Fix generated route name not correctly detected when using prefixes Jan 21, 2021
@stayallive
Copy link
Collaborator

Hey @wfeller I have been testing this but I don't see this problem occurring...

I am testing on Laravel 8, which version are you using?

When using the route below and running php artisan route:cache I get: generated::FRnHID1MBC9OnhFk and not test.generated::FRnHID1MBC9OnhFk.

Route::group([
    'name' => 'test.',
], function () {
	Route::view('test', 'home');
})

@wfeller
Copy link
Contributor Author

wfeller commented Jan 21, 2021

We're on Laravel 8.23.1

Try it with this maybe - I think the name argument doesn't do the same thing as prefix:

Route::prefix('prefix.')->group(function () {
    Route::view('test', 'home');
});

From my Sentry performance tab:
image

@stayallive
Copy link
Collaborator

I don't think this does what I think it would do...

Route::prefix('pizza.')->group(function () {
    Route::view('test', 'home');
});

This generates the route: app.develop/pizza./test, I don't think that is what you intended?

Below generates the route: app.develop/test as I would expect.

Route::name('pizza.')->group(function () {
    Route::view('test', 'home');
});

Maybe I'm totally missing the boat here and you mean to use the prefix that way but I don't think that's right.

But even with that inspecting my bootstrap/cache/routes-v7.php I cannot get Laravel to generate the prefix before the generated name :(

@wfeller
Copy link
Contributor Author

wfeller commented Jan 22, 2021

My bad, I looked at my routes file and saw prefix first, thought it had to be it! (we actually use both prefix and as)

I can reproduce it in a fresh Laravel codebase:

Route::as('prefix.')->group(function () {
    Route::get('test-1', 'Controller@action');
    Route::get('test-2', 'Controller@action');
});

This will generate 2 cached routes:
"prefix.": /test-1
"prefix.generated::U282gBpR9Z1vbfVc": /test-2

It doesn't seem possible to reproduce the issue with only 1 unnamed route.

@stayallive stayallive merged commit a5439f2 into getsentry:master Jan 23, 2021
@stayallive
Copy link
Collaborator

Yep that did it! Sorry about the many questions just wanted to make sure this was needed.

Thank you for your patience, I hope to release this in the next week.

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.

Generated route name not correctly detected when using prefixes
2 participants