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

Response construct use property promotion #1050

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

joanhey
Copy link
Contributor

@joanhey joanhey commented Aug 6, 2024

Also fix a problem with the last __construct, that permit array|null in $headers and later this will break.

Before:

protected array $headers = [];

    /**
     * Response constructor.
     *
     * @param int $status
     * @param array|null $headers
     * @param string $body
     */
 public function __construct(
        int     $status = 200,
        ?array  $headers = [],
        string  $body = ''
    )
    {
        $this->status = $status;
        $this->headers = $headers;
        $this->body = $body;
    }

$this->headers = $headers; will break if we send a null.

And if we permit null, we have more problems in the class.

Example:

public function getHeaders(): array
    {
        return $this->headers;
    }
...

@joanhey
Copy link
Contributor Author

joanhey commented Aug 6, 2024

Curiously was passing the tests, than use null. 😮

@walkor you can decide how do you want it.

We can also check in the constructor if is a null to be converted to array().

EDIT: the fail is from the static analyzer phpstan, not the unit tests

@joanhey joanhey changed the title Response construct use propery promotion Response construct use property promotion Aug 7, 2024
@walkor walkor merged commit fea0569 into walkor:master Aug 8, 2024
18 checks passed
@walkor
Copy link
Owner

walkor commented Aug 8, 2024

I agree with your PR

@joanhey joanhey deleted the response-property-promotion branch August 8, 2024 06:41
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.

2 participants