Conversation
|
As far as I recall, Taylor doesn't deem it a necessary inclusion, hinting at |
But what IF he has a change of heart due to the beautiful code I add???? |
|
That would indeed be great 🤞🏻 Just didn't want you to get your hopes up 😉 |
| } | ||
|
|
||
| /** | ||
| * Cast an "object" builtin value into a stdClass instance when appropriate. |
There was a problem hiding this comment.
You can narrow the return type if you want
| * Cast an "object" builtin value into a stdClass instance when appropriate. | |
| * Cast an "object" builtin value into a stdClass instance when appropriate. | |
| * | |
| * @template TValue | |
| * | |
| * @param TValue $value | |
| * @return ($value is array ? object : TValue) |
or
| * Cast an "object" builtin value into a stdClass instance when appropriate. | |
| * Cast an "object" builtin value into a stdClass instance when appropriate. | |
| * | |
| * @template TValue | |
| * | |
| * @param TValue $value | |
| * @phpstan-return ($value is array ? object : TValue) |
| * @param mixed $value The default value. | ||
| * @return mixed |
There was a problem hiding this comment.
| * @param mixed $value The default value. | |
| * @return mixed | |
| * @template TValue | |
| * | |
| * @param TValue $value | |
| * @return ($value is empty ? null : ($value is \BackedEnum ? value-of<TValue> : ($value is \UnitEnum ? string : TValue))) |
or
| * @param mixed $value The default value. | |
| * @return mixed | |
| * @template TValue | |
| * | |
| * @param TValue $value | |
| * @phpstan-return ($value is empty ? null : ($value is \BackedEnum ? value-of<TValue> : ($value is \UnitEnum ? string : TValue))) |
| /** | ||
| * Determine if the given class name is a date object type. | ||
| * | ||
| * @param class-string $name |
There was a problem hiding this comment.
You can narrow this further down if you want:
| * @param class-string $name | |
| * @template TClass | |
| * | |
| * @param class-string<TClass> $name | |
| * @return (TClass is \DateTimeInterface ? true : false) |
or
| * @param class-string $name | |
| * @template TClass | |
| * | |
| * @param class-string<TClass> $name | |
| * @phpstan-return (TClass is \DateTimeInterface ? true : false) |
| * Cast the given value to the requested date object type. | ||
| * | ||
| * @param class-string $typeName The date object class name. | ||
| * @param mixed $value The validated value. |
There was a problem hiding this comment.
You could probably add a return type here:
| * @param mixed $value The validated value. | |
| * @param mixed $value The validated value. | |
| * @return ($value is null ? null : \DateTimeInterface) |
| * @param mixed $value The validated value. | |
| * @param mixed $value The validated value. | |
| * @phpstan-return ($value is null ? null : \DateTimeInterface) |
|
I've read the original post on this a couple times now, and I'm still struggling to understand the explicit value proposition. It almost feels like you're trying to turn the Form Request into a bit of a god object and absorb even more behavior from the controller? The inference of rules feels very odd to me. So if I define something as You talk about :
Can you explain or give an example of how you would use multiple in a single controller? Could you maybe give a simple example of how you seeing this working end to end with a controller? Thanks |
|
Thanks for @browner12
I tend to disagree that this is a god object or absorbing behavior from the controller necessarily. I think it depends on how you tend to write code. If you're used to Laravel Data or Pydantic, you may have grown accustomed to being able to construct a data object from a request. It's a massive improvement on developer experience for both testing and writing clean production code. Right now, with out-of-the-box Laravel functionality, if you are using a FormRequest but want some sort of typed object, you end up having to write at least two classes: the FormRequest itself and then a typed POPO to map it into. You may also be of the ilk that writes a mapper for this, making three classes. Again using Laravel's out-of-the-box experience, you may tuck these into separate namespaces based on their type. This means you end end up having to jump from Controller to FormRequest to data object, and it's very easy for one of these to drift and not be caught by an integration test. (Ever brought down prod because you added a new validation rule but forgot to include it in the DTO mapping? Or changed an input field and forgot to update the DTO mapping?) Mapping inside of the controller makes testing harder still, as you now can only test your FormRequest through a controller test. Imagine your request lives in a route like
Yes, I think the next step is adding docblock parsing so we can have You can of course just drop the rule inference if you don't like it (or not use these TypedFormRequests at all, of course).
Sure! Imagine an endpoint that receives a payload like this: {
"order_id": 1345,
"address": {
"street1": "123 Main St",
"city": "Los Angeles",
"state": "CA",
"postal_code": "90210"
}
}You could create a TypedFormRequest like this: class Address
{
public function __construct(
public string $street1,
public string $city,
public string $state,
#[MapFrom('postal_code')]
public string $postalCode,
public ?string $street2 = null,
#[MapFrom('country_code')]
public ?string $countryCode = 'US',
) {}
}
class SetOrderRequest extends TypedFormRequest
{
public function __construct(
#[MapFrom('order_id')]
public int $orderId,
public Address $address,
) {}
}You'll see we have a typed form request that contains a sub-object.
class SetOrderController
{
public function __invoke(
SetOrderRequest $request,
#[CurrentUser] User $user,
AddressRepository $addressRepository,
SetOrderAction $action
) {
$addressRepository->updateOrCreateAddress($request->address, $user);
$action->handle($request, $user);
return response()->noContent();
}
}Hope that helps! |
|
That definitely helps, thank you for the explanation. I'll try to digest the idea a little more, but one thing I can say is I am definitely against the "parsing of docblocks" to infer validation rules. My gut reaction is it feels very odd to me to elevate these request objects to first-class citizens of your Domain Layer. If the goal is you're trying to map HTTP request data to your Domain Layer, isn't that your Models? Looking at the controller example you gave, I'd propose something like this: class SetOrderController
{
public function __invoke(
FormRequest $request,
AddressRepository $addressRepository,
SetOrderAction $action,
) {
$address = $addressRepository->updateOrCreateAddress([
$request->validated('street'),
$request->validated('city'),
$request->validated('state'),
]);
$action->handle($address);
return response()->noContent();
}
}I'm struggling to see the benefit of this extra intermediary DTO. The other thing that feels odd to me is now you're coupling your Actions to the typed form requests. There's obviously the type safety benefit you're trying to get with this PR, but there's likely also a re-usability benefit you're aiming for. What happens if multiple endpoints trigger this action? Do they all now need to submit the data with the exact same field names, and the exact same validation rules to generate the Typed Form Request the action is expecting? If I'm missing something, happy to learn. |
Sure, that's totally reasonable and a lot of legacy code relies on it. I work with a lot of code that looks like this, and it certainly works. Coming up with toy examples is always a pain in the ass, and no matter what anyone chooses, someone says "but you could do it this way" (and they're totally correct in that). My hope was to illuminate an idea on how it could be used, not to be prescriptive about how you must use it.
I agree that coupling a service method to a request object isn't something I would recommend in most cases. But I have also come to recognize that most of my service layer code gets executed by one branch of production code and then a whole bunch by tests. Sometimes it gets reused, but that's usually an exception rather than the norm. If I can have one request object, one controller, and one service layer function, then that's great. I have good testability. I don't need to write an extra DTO and a mapper. The nice thing about these TypedFormRequests is that they are easy to instantiate. I can just as easily write Having I don't think there is much debate that the typical developer is moving towards greater type-safety in their code. 🤷 LLMs benefit from it, tooling benefits from it, developers have to keep less stuff in their heads, it reduces mapping boilerplate, and it's easier to convert to TypeScript definitions. I think your concerns about the dependency direction are totally fair. If someone chooses to use this, there's nothing preventing them from treating it as strongly-typed, validated data to build another data object. Others may appreciate the ergonomics of passing this object directly to another layer without that mapping step. The core value here is collapsing what today requires three classes (FormRequest + DTO + mapper) into one, while still being easy to instantiate outside of a request lifecycle when you need to. You keep the type safety, lose the boilerplate, and reduce the surface area for drift bugs between validation rules and data structures. |
|
My two cents: Personally not a big fan of magic/inference when it comes to stuff like validation, it becomes easy to mess something up and since validation errors aren't reported by default it becomes very easy to miss bugs. I think being able to represent an HTTP request as an injectable DTO is interesting, but request validation should probably be explicit (IMHO). I also wonder if there is significant benefit of having this in the framework, vs. as a package. That said, if this does move forward, does it maybe make more sense to use an interface (similarly to how FormRequests work?) That would allow a bit more flexibility such as extending another (unrelated) class or making the class readonly, both of which aren't possible with a base class. Finally, I might've missed it, but it seems file uploads aren't supported at all right now. |
Important point 👍🏻
That has always bugged me as well
In the end, the both above points show, that this is essentially one process in most cases. Having it broken down in an explicit two-step process is a little cumbersome in many cases.
@axlon From the description, @cosmastech's PR offers
so validation can still work like it does if users prefer that. And not breaking validation is an essential goal for this PR, I'd say 👍🏻 |
|
Thanks for the comments @axlon
I'll have to revisit that idea. Not sure if that didn't work with my initial plan, or I just wasn't thinking of that.
|
d2ae357 to
200c6fe
Compare
200c6fe to
1c15fdb
Compare
|
Hey @cosmastech - imo the TypeFormRequestFactory is really long. I wonder if some of the general areas of functionality can be broken out into some Concerns / Traits to make it a bit easier to reason about? I had done this on a local branch already but you've pushed a lot of commits then so it's all out of sync. |
Sure @taylorotwell. What were the names of the traits had you broken them into? That will help steer me. 🙇 |
|
I'm going to stop tinkering with the branch. It's all yours @taylorotwell 🫡 |
|
I'd like to add a few things to the discussion:
|
|
|
I agree that adding a plugin mapping/casting system would be great. interface RequestCastable
{
public function rules(/* TBD params */): array;
public function cast(/* TBD params */): mixed;
}We could create a static array of default casts, as well as a method on the TypedFormRequest child class that could accept overrides as well. @taylorotwell let me know if you want me to add that before you start formatting commits. Mapping all to snake case or whatever would be good. That doesn't feel like a "must-have" for this version IMO, but could follow up before the 13.x release. As for Optional types for PATCH requests.... I wonder if we couldn't just get there with the RequestCastable, perhaps? edit: Should we add a new namespace just for the TypedFormRequest stuff? |

Allow userland to use typed-data objects for requests, rather than using the FormRequest.
Why
LLMs like types. I like types. PHPStan likes types. TypeScript like types. Accessing data from a FormRequests feels incredibly icky and is error prone. FormRequests often times end up being mapped to DTOs anyways in order to pass them to a service or action.
But what about Spatie's laravel-data package?
It's great. It's also kind of heavy weight. It's not first-party and doesn't get the contributor love that Laravel's framework does. It makes it harder to move to new projects because generally you have to make the case for why Laravel Data is the shit. First-party => no problem, run it. It also doesn't necessarily have the same level of interop that I believe a first-party solution would.
What about...?
You tell me what you feel needs added to make this dream a reality and I will add it.
Why Laravel 13?
I happened to start with master because I had planned to use some of the attributes you were already intending to add. Plus I had a dream of adding more attributes like
#[Rules]and stuff.Ok, so I'm sorta convinced, tell me about what this offers
Glad you're coming around!
sometimes|int#[WithoutInferringRules]attribute to your classfirstName)