saloonphp / saloon

🤠 Build beautiful API integrations and SDKs with Saloon
https://docs.saloon.dev
MIT License
2.03k stars 105 forks source link

Request class cannot accept `$query` property in constructor #427

Closed derrickobedgiu1 closed 2 months ago

derrickobedgiu1 commented 2 months ago

Hello, I was trying to build a request body but being locked down on overwriting non compatible types.

<?php
// code
class TheRequest extends Request implements HasBody{
use HasJsonBody;

protected Method $method = Method::POST;

public function __construct(
        protected string $name,
        protected string $query,
)
{
}
// code
}

Error: Type must be '\Saloon\Contracts\ArrayStore' (as in base class '\Saloon\Traits\RequestProperties\HasQuery')

PhpStan: Type string of property Namespace\Requests\TheRequest::$query is not the same as type Saloon\Contracts\ArrayStore of overridden property Saloon\Http\Request::$query.

Is it that I CANNOT use $query property in my request constructor or am missing something? Bypassing this by assigning myself another property name (eg $qry) besides $query looks nasty as am following an API Spec and didn't intend to go this route.

Partial code provided but request is complete with defaultBody defined

patrickcarlohickman commented 2 months ago

@derrickobedgiu1,

The Request uses the HasRequestProperties trait, which in turn uses the HasQuery trait, which defines the $query property on the class: protected ArrayStoreContract $query;.

Because the Request class already has a $query property defined, your constructor promotion is attempting to redefine it as a different, incompatible type.

Since your class already has the $query property defined (as a different type), you'll either need to rename your constructor parameter (as you've discovered), or you'll need to remove the protected keyword to remove the property promotion. Either way, you'll still need to convert your incoming query string to a query array as used by Saloon.

Two options:

  1. Use a different parameter name and define the defaultQuery() method to use it convert it to the query string to the expected array.
class TheRequest extends Request implements HasBody
{
    use HasJsonBody;

    protected Method $method = Method::POST;

    public function __construct(
        protected string $name,
        protected string $queryString,
    ) {
        //
    }

    protected function defaultQuery(): array
    {
        // @see https://www.php.net/manual/en/function.parse-str.php
        parse_str($this->queryString, $queryArray);

        return $queryArray;
    }
}
  1. Remove the property promotion so you're not attempting to redefine the property and just set the internal $query array inside the constructor:
use Saloon\Repositories\ArrayStore;

class TheRequest extends Request implements HasBody
{
    use HasJsonBody;

    protected Method $method = Method::POST;

    public function __construct(
        protected string $name,
        string $query,
    ) {
        // @see https://www.php.net/manual/en/function.parse-str.php
        parse_str($query, $queryArray);

        $this->query = new ArrayStore($queryArray);
    }
}
juse-less commented 2 months ago

@derrickobedgiu1 You can't use properties (promotional or not) named headers, query, or body, without following the type already defined in the base request. There's also some other properties, but these are the common 'message'/request terms.

Other than what @patrickcarlohickman has already suggested, you can also use DTOs like @Sammyjo20 mentions here: https://github.com/saloonphp/saloon/issues/376#issuecomment-1962975230.

In my experience, DTOs works quite well, and are easy to serialise and unserialise in-between processes (like queueing Laravel jobs). It works fairly well with API specs, but might get a bit tricky if you use a tool to generate things based on something like OpenAPI. Although, there are tools to do it. F.ex., Saloon SDK Generator, that's specifically made for Saloon. Although, I haven't gotten around to test- or dig into it yet. So I'm not entirely sure what's possible. You could also use something more PHP-generic like JanePHP, although this likely takes a bit more effort.

derrickobedgiu1 commented 2 months ago

Hello @patrickcarlohickman & @juse-less , thanks for responses. I'll go ahead and use a different property name. Thanks for the time