search

area17 / twill

Twill is an open source CMS toolkit for Laravel that helps developers rapidly create a custom admin console that is intuitive, powerful and flexible. Chat with us on Discord at https://discord.gg/cnWk7EFv8R.
https://twillcms.com
Apache License 2.0
3.63k stars 560 forks source link

Default login redirect to admin url #2569

Closed Tofandel closed 1 month ago

Tofandel commented 3 months ago

Fixes #2568

ifox commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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