Skip to content

[5.6] Allow nullable method injection#22488

Closed
mpyw wants to merge 1 commit intolaravel:masterfrom
mpyw-forks:nullable-dependency-injection
Closed

[5.6] Allow nullable method injection#22488
mpyw wants to merge 1 commit intolaravel:masterfrom
mpyw-forks:nullable-dependency-injection

Conversation

@mpyw
Copy link
Copy Markdown
Contributor

@mpyw mpyw commented Dec 20, 2017

I wrote the following controller action that can accept multiple routes. Some of injected arguments are nullable.

Route::get('articles/{article}/comments', 'CommentController@index');
Route::get('events/{event}/comments', 'CommentController@index');
class CommentController
{
    public function index(Request $request, Article $article = null, Event $event = null)
    {
        $commentable = $article ?: $event;
        return $commentable->comments();
    }
}

However, Laravel currently cannot handle null value injection. This PR makes it possible.


Related: 【Laravel】Route Model Binding と Policy だけで認可処理を完結させるための Resourceful ルーティングの工夫 - Qiita

@Dylan-DPC-zz
Copy link
Copy Markdown

This is a breaking change. It is better to PR it to master (5.6)

@mpyw mpyw changed the title [5.5] Allow nullable method injection [5.6] Allow nullable method injection Dec 20, 2017
@mpyw mpyw changed the base branch from 5.5 to master December 20, 2017 05:33
@mpyw
Copy link
Copy Markdown
Contributor Author

mpyw commented Dec 20, 2017

Thank you for responding.

I really want the feature like Polymorphic Route Model Binding. I hope it will be supported in the future version of Laravel.

@mpyw mpyw changed the title [5.6] Allow nullable method injection [5.6] Allow nullable method injection (Polymorphic Route Model Binding) Dec 20, 2017
@sisve
Copy link
Copy Markdown
Contributor

sisve commented Dec 20, 2017

And where are the tests?

@mpyw
Copy link
Copy Markdown
Contributor Author

mpyw commented Dec 20, 2017

Currently not yet. And I don't know how should I treat broken tests...

@sisve
Copy link
Copy Markdown
Contributor

sisve commented Dec 20, 2017

There's nothing in this PR that is polymorphic, nor does it change the model binding. Don't call it that.

If you really wanted something polymorphic, make it work with a Commentable interface that both Article and Event implements. Then use that in your action. public function index(Request $request, Commentable $commentable) { ... }

Note that this change broke existing tests. That is a really bad sign. You need to modify your change so that it at least doesn't break existing code, and add tests that shows the new behavior.

@sisve
Copy link
Copy Markdown
Contributor

sisve commented Dec 20, 2017

To clarify; you are not allowed to change any existing tests. They exist to make sure that things aren't broken. You have to change your code so that the existing tests keeps working.

@mpyw mpyw changed the title [5.6] Allow nullable method injection (Polymorphic Route Model Binding) [5.6] Allow nullable method injection Dec 20, 2017
@mpyw
Copy link
Copy Markdown
Contributor Author

mpyw commented Dec 20, 2017

If you really wanted something polymorphic, make it work with a Commentable interface that both Article and Event implements. Then use that in your action.

I already tried to do that, however, how can I define binding rule for each route?

@sisve
Copy link
Copy Markdown
Contributor

sisve commented Dec 20, 2017

This isn't a good place to ask about these things. I suggest that you close this PR and ask in the chat at https://larachat.co/ or start a discussion at https://github.com/laravel/internals

@mpyw
Copy link
Copy Markdown
Contributor Author

mpyw commented Dec 20, 2017

Okay

@mpyw mpyw closed this Dec 20, 2017
@mpyw mpyw deleted the nullable-dependency-injection branch December 20, 2017 06:59
@mpyw
Copy link
Copy Markdown
Contributor Author

mpyw commented Dec 20, 2017

Finally I got the solution by overriding Router, Route and ControllerDispatcher.

[Laravel] ルーティングサービスを継承する - Qiita

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