Skip to content

Conversation

@VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Jul 1, 2025

Q A
Branch? main for features
Tickets Closes #..., closes #...
License MIT
Doc PR api-platform/docs#...

Comment on lines +118 to +120
identifier: missingType.iterableValue
-
identifier: missingType.generics
Copy link
Contributor Author

Choose a reason for hiding this comment

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

iterableValue and generics are creating a lot of error, so I prefer to fix missing param/property/return type first

-
identifier: missingType.property
paths:
- src/Doctrine/Common/Tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ignored all the tests folder.

- src/Symfony/Bundle/Test
- src/Symfony/Tests
- tests
- src # TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parameter is ~150 error, I'll do it in the next PR.

paths:
- src/Symfony/Bundle/Test
- tests
- src # TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's around 50 error, I'll try in another PR.

}

private function buildArrayValue(?\SimpleXMLElement $resource, string $key, mixed $default = null)
private function buildArrayValue(?\SimpleXMLElement $resource, string $key): ?array
Copy link
Contributor Author

Choose a reason for hiding this comment

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

default is never used

@VincentLanglet
Copy link
Contributor Author

@soyuka The PHPStan error seems legit to me

 ------ ----------------------------------------------------------------------- 
  Line   Exception/ErrorHandler.php                                             
 ------ ----------------------------------------------------------------------- 
  80     Argument of an invalid type array<int|string,                          
         array<string>|string>|string supplied for foreach, only iterables are  
         supported.                                                             
         🪪 foreach.nonIterable 

HttpOperation accepts the following param

     * @param array<int|string, string|string[]>|string|null $formats       {@see https://api-platform.com/docs/core/content-negotiation/#configuring-formats-for-a-specific-resource-or-operation}
     * @param array<int|string, string|string[]>|string|null $inputFormats  {@see https://api-platform.com/docs/core/content-negotiation/#configuring-formats-for-a-specific-resource-or-operation}
     * @param array<int|string, string|string[]>|string|null $outputFormats

but then we're doing foreach on it.

If someone pass a string instead of string[] you'll get a weird behavior.
Not sure what solution you prefer:

  • Casting the format into an array (in the constructor ? in the getter ?)
  • Casting the format when iterating on it (but it's not the only place)
  • Updating the param to only accept array|null and not string

@soyuka
Copy link
Member

soyuka commented Jul 1, 2025

Indeed, to me we should cast the string into an array, the foreach should be changed to op->getOutputFormats() ?? null and the getter should only return:

list<string>, array<string, list[string]>|null

@VincentLanglet VincentLanglet marked this pull request as ready for review July 1, 2025 15:51
$uriVariables = [];
foreach ($operation->getUriVariables() ?? [] as $parameterName => $_) {
$parameter = $request->route($parameterName);
$parameter = $request->route((string) $parameterName);
Copy link
Member

Choose a reason for hiding this comment

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

mhh actually this is weird, it's a hard problem though maybe we'll leave that for later ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an error saying that route is called with int|string instead of string, so I casted the value.
This value is also casted on the following line.

You prefer a phpstan-ignore-line ?

*
* @return string[]
*/
public function getIris()
Copy link
Member

Choose a reason for hiding this comment

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

can we change the return type to ?array as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, done with c29eb43

*
* @return bool
*/
public function getGenId()
Copy link
Member

Choose a reason for hiding this comment

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

same we can return ?bool here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, done with c29eb43

@soyuka soyuka merged commit cb60937 into api-platform:main Jul 3, 2025
113 of 114 checks passed
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