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

Addition of ConfirmPasswordController.php breaking change #30382

Closed
danabrey opened this issue Oct 22, 2019 · 11 comments · Fixed by #30389
Closed

Addition of ConfirmPasswordController.php breaking change #30382

danabrey opened this issue Oct 22, 2019 · 11 comments · Fixed by #30389

Comments

@danabrey
Copy link

  • Laravel Version: 6.2
  • PHP Version: 7.3
  • Database Driver & Version:

Description:

This issue involving missing ConfirmPasswordController occurs when upgrading from Laravel 6.0 to 6.2. It's not a backwards-compatible new feature, and requires adding new files to the application codebase to avoid errors.

Doesn't this break the idea of semantic versioning? Regardless, I am unable to find an 'upgrade guide' for 6.0 to 6.1 or 6.1 to 6.2 - maybe I'm missing them? Guides like this shouldn't be as necessary for a project that respects semver, but if changes like this are occurring they become crucial.

Steps To Reproduce:

  • Create a Laravel 6.0 project
  • Upgrade to Laravel 6.2
@alec-joy
Copy link

Where are you getting this error message? We're on Laravel 6.3, upgraded from 5.8 (all the way back to 5.4 actually) and although we don't have this controller, we don't get this error anywhere

@devcircus
Copy link
Contributor

Since the 6.3 release I’ve only seen 2-3 devs mention problems. Has to be something else going on. The repo would be flooded with issues if this was a problem.

@alec-joy
Copy link

alec-joy commented Oct 22, 2019

Okay I think I see the issue, our app doesn't use the Auth::routes(); helper as we have our own auth logic. If you use Auth::routes() I do get this error. That's the extra step.

Edit: The method defaults to adding the route for the confirmPassword controller so yes this would be a breaking change for anyone who uses the Auth::routes() helper and upgrades from a previous version https://github.com/laravel/framework/blob/6.x/src/Illuminate/Routing/Router.php#L1167

@devcircus
Copy link
Contributor

Interesting. If true, it’s something that should not have been added if Laravel is serious about semver.

@Tantrisse
Copy link

Hi !
Same problem here, got the error after upgrading from 6.1 to 6.3 and it was because I use

Auth::routes([
    'reset'    => false,
    'verify'   => false,
    'register' => false,
]);

in my web.php route file.
I had to add confirm => false to fix it.

@browner12
Copy link
Contributor

Let's get rid of Auth::routes() and force users to be explicit about their routes.

@ajoy39
Copy link
Contributor

ajoy39 commented Oct 22, 2019

That might be an option long term but it would be a breaking change and 6.x is an LTS version of laravel, so it wouldn't help solve the core issue. There either needs to be a fallback in the app or a conditional around registering those routes if the file doesn't exist

@browner12
Copy link
Contributor

yah, I'd probably be tarred and feathered if I actually made that suggestion.

wrapping in conditionals is tricky, because are you just checking to see if the controllers exist? or are you also checking to make sure the methods exist on those controller? and all that logic seems like a waste.

@netpok
Copy link
Contributor

netpok commented Oct 22, 2019

Actually this only breaks the route:list command and the newly introduced endpoints as far as I know, I tried route:cache, it's not affected.

That being I don't think this feature should have been added enabled by default, actually I think this should be disabled by default, just like the verify option.

Sadly, I think disabling it after a week would probably cause more breaks in production instances than leaving it.

This definitely should be considered next time.

@browner12
Copy link
Contributor

I'm a glutton for punishment.... #30388

@netpok
Copy link
Contributor

netpok commented Oct 22, 2019

While I could live with that I see virtually 0% chance for that getting merged.

I sent a pull request to alleviate these breaks.

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 a pull request may close this issue.

7 participants