Skip to content

Feature/upgrade libraries #19

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jama-loma
Copy link

psr/http-message was upgraded to 2.0, so was phpstan. This change makes it possible to require this library w/o conflicts in composer

@@ -29,7 +29,7 @@ public function getRawBody(): string

private function getUri(): string
{
return $_SERVER['REQUEST_URI'] ?? '/';
return $_SERVER['REQUEST_URI'] ?? '/'; // @phpstan-ignore return.type
Copy link
Member

Choose a reason for hiding this comment

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

What was the phpstan error here? Can we rather fix it?

Copy link
Author

Choose a reason for hiding this comment

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

$_SERVER['REQUEST_URI'] (like everything in $_SERVER) is considered mixed. Fixable, like the others, at the cost of some readability. Can do.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, pls put there some check. Since it can be mixed... thanks

@@ -41,13 +41,15 @@ private function getHttpMethod(): string
}

if ($method === 'POST' && isset($_SERVER['HTTP_X_HTTP_METHOD_OVERRIDE'])) {
$matched = preg_match('#^[A-Z]+\z#', $_SERVER['HTTP_X_HTTP_METHOD_OVERRIDE']);
$matched = preg_match('#^[A-Z]+\z#', $_SERVER['HTTP_X_HTTP_METHOD_OVERRIDE']); // @phpstan-ignore argument.type
Copy link
Member

Choose a reason for hiding this comment

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

The same here.


if ($matched === 1) {
$method = $_SERVER['HTTP_X_HTTP_METHOD_OVERRIDE'];
}
}

assert(is_string($method));
Copy link
Member

Choose a reason for hiding this comment

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

Are we using asserts in this library at all? If not, what is the reason for it?

Copy link
Author

Choose a reason for hiding this comment

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

No. Fixing the whole class makes this check useless, removed.

@@ -30,7 +30,7 @@ public function create(string $rawRequest): RequestCollection
if (is_array($requestData)) {
foreach ($requestData as $oneRequestData) {
$collection->attach(
$this->createRequestFromRequestData($oneRequestData)
$this->createRequestFromRequestData($oneRequestData) // @phpstan-ignore argument.type
Copy link
Member

Choose a reason for hiding this comment

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

Phpstan is here to catch edge cases before there is a runtime error. We shouldn't really just ignore everything it throws at us.

@jama-loma jama-loma force-pushed the feature/upgrade-libraries branch from fa0d19f to afacaa0 Compare March 7, 2025 14:35
@jama-loma
Copy link
Author

jama-loma commented Mar 7, 2025

Updated to pass PhpStan's 2.0 new inspections

  • specifically, the stricter rules against mixed.
  • now we access $_SERVER through a wrapper method, which guarantees a ?string.

@jama-loma jama-loma requested a review from paveljanda March 19, 2025 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants