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

Paths override with dot in route name does not work #500

Closed
sebj54 opened this issue Oct 17, 2023 · 3 comments · Fixed by #501
Closed

Paths override with dot in route name does not work #500

sebj54 opened this issue Oct 17, 2023 · 3 comments · Fixed by #501

Comments

@sebj54
Copy link
Contributor

sebj54 commented Oct 17, 2023

Fortify Version

1.18.0

Laravel Version

10.16.2

PHP Version

8.2.8

Database Driver & Version

No response

Description

When there is a dot in the route name, it is not possible to customize route paths the way it is described in the default config:

'paths' => [
'login' => null,
'logout' => null,
'password.request' => null,
'password.reset' => null,
'password.email' => null,
'password.update' => null,
'register' => null,
'verification.notice' => null,
'verification.verify' => null,
'verification.send' => null,
'user-profile-information.update' => null,
'user-password.update' => null,
'password.confirm' => null,
'password.confirmation' => null,
'two-factor.login' => null,
'two-factor.enable' => null,
'two-factor.confirm' => null,
'two-factor.disable' => null,
'two-factor.qr-code' => null,
'two-factor.secret-key' => null,
'two-factor.recovery-codes' => null,
],

RoutePath will get the config value for a given path with the config() function (see below). Because of that, dots are representing config paths and each item before a dot is an array.

return config('fortify.paths.'.$routeName) ?? $default;

Instead, route paths should be declared with arrays actually:

return [
    'paths' => [
        'login' => null,
        'logout' => null,
        'password' => [
            'request' => null,
            'reset' => null,
            'email' => null,
            'update' => null,
            'confirm' => null,
            'confirmation' => null,
        ],
        'register' => null,
        'verification' => [
            'notice' => null,
            'verify' => null,
            'send' => null,
        ],
        'user-profile-information' => [
            'update' => null,
        ],
        'user-password' => [
            'update' => null,
        ],
        'two-factor' => [
            'login' => null,
            'enable' => null,
            'confirm' => null,
            'disable' => null,
            'qr-code' => null,
            'secret-key' => null,
            'recovery-codes' => null,
        ]
    ],
];

The easiest fix is to change the default config object (and maybe the docs). But I think it would be more relevant if RoutePath::for() would get the whole paths array with config('fortify.paths') and then get the custom path if it exists.

Steps To Reproduce

Define a custom path for a route with a dot (for instance two-factor.login):

// config.fortify.php
return [
    'paths' => [
        'two-factor.login' => '/custom/path',
    ],
];

Then get route list with php artisan route:list

👉 The custom path is not taken in account

@driesvints
Copy link
Member

I think your suggestion to update the config to nested arrays is the right one. Can you PR this?

@github-actions
Copy link

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@sebj54
Copy link
Contributor Author

sebj54 commented Oct 17, 2023

Sure! :) It's done with #501

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.

2 participants