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

Default login redirect to admin url #2569

Merged
merged 1 commit into from May 26, 2024
Merged

Conversation

Tofandel
Copy link
Contributor

@Tofandel Tofandel commented Apr 1, 2024

Fixes #2568

@Tofandel Tofandel force-pushed the fix/auth-default-url branch 3 times, most recently from 268ad72 to f35602c Compare April 2, 2024 00:23
@ifox
Copy link
Member

ifox commented May 23, 2024

Thanks @Tofandel, I agree however this PR doesn't actually change the default config value, right? We can document that it can be set to null for now to get the new default behavior and make the config breaking change in Twill 4.

@Tofandel
Copy link
Contributor Author

No it doesn't, I forgot about it, the config is set by default there https://github.com/area17/twill/blob/3.x/config/twill.php#L175, I and so this PR doesn't apply unless explicitely set to null, don't think changing it to null would be considered a breaking change would it?

@ifox
Copy link
Member

ifox commented May 23, 2024

If we set it to null I'm afraid the case where one is using both an admin domain and a path at the same time would break. It's very much an edge case but I think the mistake was to release Twill 3 with the path approach as the default without updating the configuration, so we could technically consider this a bug I guess.

@Tofandel
Copy link
Contributor Author

admin domain and a path at the same time

Not sure I understand the edge case? If we set just twill.auth_login_redirect_path to null, all the other instances of usage are already replaced by the PR, so then it just uses the new default behavior

@ifox
Copy link
Member

ifox commented May 24, 2024

You're right.

Do we need a new helpers class for this though? I'm curious why you didn't put this under TwillRoutes.

@Tofandel
Copy link
Contributor Author

Tofandel commented May 24, 2024

TwillRoutes is a good place for it, I didn't see it, I just wanted to create a Route helper to prepare for the moving of the https://github.com/area17/twill/blob/3.x/src/Helpers/routes_helpers.php to it, but I guess since there is only one method in it, it can also move to that class, which already fits well for that purpose

@ifox ifox merged commit a3713ed into area17:3.x May 26, 2024
13 checks passed
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.

Redirected to '/' on reset password
2 participants