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] Adds support for int backed enums to implicit Enum route binding #51029

Merged

Conversation

monurakkaya
Copy link
Contributor

@monurakkaya monurakkaya commented Apr 11, 2024

This pull request allows you to bind int backed enums to your routes.

// Assuming the following `Enum`
enum DocumentType: int
{
    case ANNUAL_WORK_PLAN = 1;
    case ANNUAL_TRAINING_PLAN = 2;
}

// And assuming the following route;
Route::get('/documents/{documentType}', function (DocumentType $documentType) {
    return $documentType->name;
});

now, if we try to bind int backed enum to route, we are getting the error below :

Illuminate\Contracts\Container\BindingResolutionException

Target [App\Enums\DocumentType] is not instantiable.

With this PR we'll see:

// GET `/documents/1` returns `ANNUAL_WORK_PLAN`...
// GET `/documents/2` returns `ANNUAL_TRAINING_PLAN`...
// GET `/documents/3` returns `404 Not Found`...
// GET `/documents/test` returns `404 Not Found`... <- I'm not sure if we should give 404 here

There is a comment about why we didn't do it in the first place but I don't see any reason to not to try that.

We can skip checking the backed enum type in the ReflectionEnum part and validate all values via collect($backedEnum::cases)->first() without a match

And also I've created a PR for the docs

Best.

@monurakkaya monurakkaya marked this pull request as draft April 11, 2024 20:17
@monurakkaya monurakkaya force-pushed the allow-non-string-backed-enums branch from 61ab22e to ede3d6a Compare April 11, 2024 20:37
@monurakkaya monurakkaya force-pushed the allow-non-string-backed-enums branch from ede3d6a to 8c7f247 Compare April 11, 2024 20:39
@monurakkaya monurakkaya marked this pull request as ready for review April 11, 2024 20:48
@taylorotwell taylorotwell merged commit 4341063 into laravel:11.x Apr 12, 2024
28 checks passed
@driesvints
Copy link
Member

This seems to have broken some apps: #51114

@taylorotwell
Copy link
Member

Sorry @monurakkaya - looks like this will have to go to master branch for Laravel 12

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.

3 participants