Move destination overrides in a `to()` method
Gummibeer opened this issue · 15 comments
In the current API I have to override the path with the same method I also adjust the payload. This feels wrong as changing the path is first something super rare as it's against the principle of that package. And You don't do the request with a path but to a path.
So an API like to(path: '/foo/bar')
feels better. This could also allow adjusting the fragment and other URI segments.
I definitely think there is a lot that could be done in the __callStatic
approach I currently set up - would happily accept a PR on this, or some guidance on what a friendlier DX would be
The __callStatic()
wouldn't need any adjustment. Just a new to(...)
method on the class implementing the with()
one.
The magic method will automatically do everything.
CreateUserRequest::to(path: 'foo/bar')->with(payload: ['foo' => 'bar'])->send();
I'd like to be able to expand, and maybe even drop the magic method eventually. I am imagining:
class CreateUserRequest
{
public function to(string $path): self
{
$this->path($path);
return $this;
}
public static function __callStatic($name, $args): self
{
// changes here
}
}
Refactoring how we forward calls to drop the requirement of named arguments will be a benefit for PHP 7.4 support - however it will all depend on the approach taken
The problem is that the to()
and with()
method have to be callable as static method or on an instance. Otherwise you will have to construct the request manually.
Which also means that you have to implement them twice as all the methods are on Transporter
and not the request right now.
perhaps I can do something in the call static method? If you have any ideas I'd love to hear/see them - I haven't used call static much, and also want to start moving things towards constants over floating string comparisons
So the current problem with __callStatic
and PHP7.4 is that you depend on Request::with(headers: [])
to get ['headers' => []]
as $arguments
in the __callStatic()
?
The solution is simple: in case the user doesn't use named arguments they have to use positional arguments.
Request::with([], ['X-Foo' => 'bar'])->send();
This works with PHP7 and also with __callStatic
.
You could help by add PHP doc-blocks to the trait defining __callStatic()
.
/**
* @method static Transporter to(string $path)
* @method static Transporter with(array $payload, array $headers)
*/
That way the IDE can provide proper autocompletion and the user also knows the order and names of arguments.
Yes... A doc-block over the methods would do the job, for people using PHP 7.4.
If you are going this route you can also change the union type from for example string|null
to ?string
and array|null
to ?array
.
Thank you for this aproach.
PS: I would still only document the PHP8 way - in case someone uses PHP7.4 they should be experienced enough to understand the IDE autocompletion and that they can't use PHP8 named arguments and have to switch to positional arguments instead.
This will still "force" people to PHP8 but allow people like @K2ouMais on legacy apps to use the package.
Any chance on submitting a PR @Gummibeer ? I want to make sure I understand exactly what you mean :)
Yeah, wait a moment.
Thanks @Gummibeer
A huge rebuild is underway thanks to the help from @Gummibeer please check the main branch as a lot of changes are coming to simplify and lean on the power of Laravel. This will not work yet - but sending requests directly will
Closing for now as this has moved to a different approach - I will create an issue for PHP 7.4 support