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

Problem with route name after update to 1.24.3 #573

Closed
MatasElectronics opened this issue Oct 23, 2024 · 10 comments · Fixed by #574
Closed

Problem with route name after update to 1.24.3 #573

MatasElectronics opened this issue Oct 23, 2024 · 10 comments · Fixed by #574
Labels

Comments

@MatasElectronics
Copy link

Fortify Version

1.24.3

Laravel Version

11.29.0

PHP Version

8.2.24

Database Driver & Version

No response

Description

After upgrading to version 1.24.3 we've gotten an error when trying to cache our routes:

 LogicException

 Unable to prepare route [login] for serialization. Another route has already been assigned name [login].

The problem is with this PR #571

On our application we've always had views turned off, because we had our own login view already. We have this view named login (just as Fortify has always done). And it has worked perfectly.
But with this PR, now the POST route for logging in is also called login. And that causes this undesirable behavior.

Maybe the PR can be amended by giving the POST route another name, like login.store or something?

Steps To Reproduce

  1. Create a new Laravel project.
  2. Install Laravel Fortify.
  3. Disable views in the Fortify config.
  4. Create your own login view and give it the name login.
  5. Run php artisan route:cache and you see the error.
@cima-alfa
Copy link
Contributor

cima-alfa commented Oct 23, 2024

I already responded to this in the PR (#571 (comment))

I believe that the naming convention used in fortify should remain the same as with views turned on (both GET and POST routes are named the same).

In my project I just simply named all my front (NextJS) routes with the front prefix, such as front.login.

Although I do understand that in existing projects this might be an annoyance, I still think the naming should remain the same whether views are disabled or not.

@MatasElectronics
Copy link
Author

I do not agree. A route name should be unique, and in the current situation, depending on a small config value, it either points to a route returning a view, or to a route to perform the actual login. I'd rather see the POST route is just named no matter what, it shouldn't depend on a "views" config value.

Also, I wouldn't expect a patch release to break our app...

@cima-alfa
Copy link
Contributor

To be fair, as far as I know both GET and POST routes has always used the same name in fortify for login, register and a few other routes. I wouldn't use names on my custom routes that might collide with official laravel route names, but I agree that this solution at this point might cause issues for existing projects.

If needed and everyone agrees, I can go ahead and submit another PR that just simply uses unique names for affected routes.

@sergey-yabloncev
Copy link

Faced the same problem after the update

@cima-alfa
Copy link
Contributor

cima-alfa commented Oct 24, 2024

Let's see, if I submit a new PR with unique names for affected routes, that shouldn't break any functionality for users with views enabled and use the GET route name to send POST requests to that route, right? Ultimately, the URL should still be the same.

Edit:
Also, as far as I know, this was a very minor update that doesn't really change any other functionality. It might be better to stay on previous version for now if this affects you.

@crynobone
Copy link
Member

I believe the PR should be reverted and only registered named route for POST request should be unique to avoid above issues. Maybe fortify.login for POST /login and so on.

@cima-alfa
Copy link
Contributor

What about the other routes? Register, password confirm and two-factor login. If you have views disabled and then create a custom route with the name password.confirm on your own, then you have the same problem as with the login route.
However, I don't think we can use a different name for the password confirm POST route because that is the name Laravel uses internally.

The problem here is not with this PR (which only fixed a bug that has been there for quite some time and was reported multiple times - guess I just provided a valid example as for why it should be fixed). The issue is actually that you shouldn't be using names on your own routes that Laravel uses internally.

Sure, I can submit a PR with a different name for the Login and Register POST routes. But ultimately, the problem you described doesn't go away for the password.confirm and two-factor.login routes.

@crynobone
Copy link
Member

The problem here is not with this PR

The issue here is that it causes breaking change, it wouldn't be an issue if it was for a major release where it doesn't break existing applications with a simple composer update.

@cima-alfa
Copy link
Contributor

cima-alfa commented Oct 28, 2024

Right, I'll make a new PR reverting to the original code and submit a new PR for the master branch with the proposed changes.

Or revert the previous PR, if possible.

cima-alfa added a commit to cima-alfa/fortify that referenced this issue Oct 28, 2024
Fix breaking change for users that use the names of the affected routes in their own route definitions.

Remove unnecessary conditions.
@cima-alfa
Copy link
Contributor

I ended up just adding the .store suffix to all the affected routes, including the password.confirm POST route as I don't think my previous concern is actually applicable here. (PR #574)

@crynobone crynobone linked a pull request Oct 29, 2024 that will close this issue
@crynobone crynobone added the bug label Oct 29, 2024
taylorotwell added a commit that referenced this issue Oct 29, 2024
* Rename POST routes (#573)

Fix breaking change for users that use the names of the affected routes in their own route definitions.

Remove unnecessary conditions.

* Update routes.php

---------

Co-authored-by: Taylor Otwell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants