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

[11.x] Allow string backed enums for route name() and domain() #51931

Closed

Conversation

NickSdot
Copy link
Contributor

We recently found ourselves adopting string backed enums to manage route names. Our app only has a few hundred named routes. But combined with n references to them, that is a lot of string-y stuff. If things get messy in our medium sized app, I can see this approach being helpful for others.

One small annoyance, however, is that we now have to call ->value on the enums a ton of times. Our code would look cleaner if we could pass the enum directly to methods like Route::name() and Redirect::route().

This PR seeks to allow this.

Proposal

enum AuthRoutes: string
{
    case LoginView = 'auth.login.view';
    // all route names here 
}

Before:

Route::name(AuthRoutes::LoginView->value)
       ->get('/', LoginViewController::class);

After:

Route::name(AuthRoutes::LoginView)
       ->get('/', LoginViewController::class);

Why Enums

  • There will be only one place where a string-y route names need to be changed. Renaming enum cases is much easier than renaming strings! When replacing route names, a simple regex will not work, and you will either end up with very complex regex rules to cover all possibilities (some of ours: $menu->route(), Form::model($model, ['method' => 'PUT', 'files' => true, 'route' => []]), route(), Redirect::route(), modal(route()), Route::name(), and most likely still not catach all. In addition, you deal with potentially conflicting strings from config(), view(), named services, middleware aliases, custom methods, facades, channels etc.). Or you go through them one by one. It's painful.
  • Backed enum values are self-validating. This makes e.g. duplicate route names, and therefore accidental overwriting, impossible.
  • Thanks to the IDE's support for enums, unused routes can be easily found and their usage determined by skimming one file with zero effort.
  • First party auto-completion.

I think it is obvious that using enums instead of strings adds a lot of value.

Notes

  • tbd: I decided to disallow integer backed enums altogether. Alternatively, they could be cast to strings. Is there any value in this?
  • Internally for us, this feature only makes sense for route names and route domains. Not sure if this is different for others.
  • I have already added a bunch of tests. I'd appreciate feedback on if more tests are required.
  • I am looking for initial feedback on the idea. If interest is confirmed, I would check again in detail to make sure that e.g. Route::has() also supports the feature, and that I did not miss anything.

@henzeb
Copy link
Contributor

henzeb commented Jun 27, 2024

What about non backed enums?

@NickSdot
Copy link
Contributor Author

What about non backed enums?

Explicitly checking for backed enums.

@henzeb
Copy link
Contributor

henzeb commented Jun 27, 2024

What about non backed enums?

Explicitly checking for backed enums.

That is not what I was hoping for. I am not that keen on typing things twice if I don't need to. I'll keep writing '->name' then.

@NickSdot
Copy link
Contributor Author

That is not what I was hoping for. I am not that keen on typing things twice if I don't need to. I'll keep writing '->name' then.

Following the same principle as it was decided for implicit route enum binding:

https://laravel.com/docs/11.x/routing#implicit-enum-binding

Personally, I think magically converting all kind of cased names for this purpose isn't the way.

Let's wait and see what Taylor thinks. 🙏

@henzeb
Copy link
Contributor

henzeb commented Jun 27, 2024

That is not what I was hoping for. I am not that keen on typing things twice if I don't need to. I'll keep writing '->name' then.

Following the same principle as it was decided for implicit route enum binding:

https://laravel.com/docs/11.x/routing#implicit-enum-binding

Personally, I think magically converting all kind of cased names for this purpose isn't the way.

Let's wait and see what Taylor thinks. 🙏

Personally I don't think we should magically convert anything. If you want lower case for some reason you can use the string backed enums, otherwise it is fine the way your enum case is defined.

@innocenzi
Copy link
Contributor

I think it would be perfectly fine if a non-backed enum like Auth::LoginView would be converted to auth.login.view.

That would avoid manual work when defining the route enums, and that would also help with keeping a convention. As @henzeb said, if you want a specific case or formatting, you could use string-backed enums.

@taylorotwell
Copy link
Member

Instead of all of this refactoring, could you just have a class with a bunch of constants?

<?php

namespace App;

class AuthRoutes
{
    public const string LoginView = 'something';
    public const string RegisterView = 'something-else';
}

@NickSdot
Copy link
Contributor Author

NickSdot commented Jun 29, 2024

@taylorotwell nah. The main benefit of enums is self-validation - duplicate values are impossible. Constants, unlike enums, don't have it. 🤷‍♂️

image

@henzeb
Copy link
Contributor

henzeb commented Jun 29, 2024

@taylorotwell nah. The main benefit of enums is self-validation - duplicate values are impossible. Constants, unlike enums, don't have it. 🤷‍♂️

Nah, with string backed enums anyone can change the value without breaking the code. Just one character uppercase is enough to be a difference.

And even constants show if they are used or not. So that is not a benefit either.

@NickSdot
Copy link
Contributor Author

#52561

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.

4 participants