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

[Laravel] Add RequestStaticValidateToInjectRector #1143

Merged
merged 1 commit into from Mar 4, 2019

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Mar 4, 2019

Ref #1138 ,
ref #290

  use Illuminate\Http\Request;
	  
  class SomeClass
  {
-     public function store()
+     public function store(\Illuminate\Http\Request $request)
      {
-         $validatedData = Request::validate(['some_attribute' => 'required']);
+         $validatedData = $request->validate(['some_attribute' => 'required']);
      }
  }

@TomasVotruba
Copy link
Member Author

@mrpixeldream Could I ask you for a review? I hope I got that right 😅

@barryvdh
Copy link

barryvdh commented Mar 4, 2019

Why just the validate? Could also be for the other Requests calls, right?

@TomasVotruba
Copy link
Member Author

Sure. Again, I don't use Laravel so I have no idea about the logic here :).

If you provide list of these magic static methods, that can be changed here, we can update it

@barryvdh
Copy link

barryvdh commented Mar 4, 2019

Ah my bad, I guess usually it's the Request facade, not the Request class itself that is called static.
I usually prefer to inject the Request class like this ($request in the controller method), but that's already done by the other call (all facades to injection in __construct). But I would say that the Request object belongs to the current controller action usually, so it would belong there. But could be a matter of preference.

@TomasVotruba
Copy link
Member Author

I'm not sure I understand. It's in controller action.

See description of this PR:

image

@barryvdh
Copy link

barryvdh commented Mar 4, 2019

This one is, but not the Facade one (that is done in #1140)

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Mar 4, 2019

Why do you talk about different PR here? I'm confused

@TomasVotruba
Copy link
Member Author

Mergin for validate method as it is now

@TomasVotruba TomasVotruba merged commit b7eae77 into master Mar 4, 2019
@TomasVotruba TomasVotruba deleted the laravel-request-to-inject branch March 4, 2019 12:15
@nschoellhorn
Copy link
Contributor

Yeah, seems fine 👍
If you want, you can check out https://github.com/laravel/framework/blob/5.8/src/Illuminate/Support/Facades/Request.php for a list of methods that are usable on the facade and hence also on the request object. The docblock shows all of them. Request::get() is frequently used as well for example. I am of course open to reviewing if you want that, just wait a little longer :P

@barryvdh
Copy link

barryvdh commented Mar 4, 2019

Because people usually call the Request facade, not the actual class. eg. Request::get() is usually an alias for Illuminate\Support\Facades\Request, not for \Illuminate\Http\Request. So those calls are already handled by the other PR (which handles all facades)

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Mar 4, 2019

The code-review would be best 👍

Copy link
Contributor

@nschoellhorn nschoellhorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest looks good, even though it seems you put something in this PR that doesn't belong there regarding the name. But that's your choice anyways! :)

TomasVotruba added a commit that referenced this pull request Nov 4, 2021
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.

None yet

3 participants