Add PSR-7 server request implementation#51
Conversation
Greptile SummaryThis PR adds
Confidence Score: 4/5Safe to merge after addressing the UploadedFile normalisation issues; the request and factory code is solid. The ServerRequest and Factory implementations are well-structured and correct. The main concerns are in UploadedFile: the OR-based isFileSpec check can silently produce a bogus UploadedFile for arrays that happen to contain a packages/psr7/src/Psr7/UploadedFile.php — the isFileSpec logic and the normalizeFiles silent-drop behaviour are the areas worth a second look before release. Important Files Changed
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/psr7/src/Psr7/UploadedFile.php:118-123
`isFileSpec` returns `true` when ANY of the three keys are present. This means an array like `['size' => 42, 'color' => 'blue']` would be treated as a file spec and passed to `normalizeFileSpec`, which would silently create `new UploadedFile('', 42, 0, null, null)` because `tmp_name` is absent. Using AND logic (requiring at least `tmp_name` + `error`) better matches the actual `$_FILES` shape and avoids creating bogus instances for incidentally matching arrays.
```suggestion
private static function isFileSpec(array $file): bool
{
return \array_key_exists('tmp_name', $file)
&& \array_key_exists('error', $file);
}
```
### Issue 2 of 3
packages/psr7/src/Psr7/UploadedFile.php:34-39
Non-array entries are silently skipped with `continue`, so they are absent from the returned array entirely. If a caller passes an `UploadedFileInterface` instance that is already normalised (a reasonable thing to do when mixing normalised and raw `$_FILES` data), it is silently lost without any error. Passing it through to the result preserves the correct tree structure in those cases.
```suggestion
foreach ($files as $key => $file) {
if ($file instanceof UploadedFileInterface) {
$normalized[$key] = $file;
continue;
}
if (!\is_array($file)) {
continue;
}
$normalized[$key] = self::isFileSpec($file)
```
### Issue 3 of 3
packages/psr7/src/Psr7/ServerRequest/Factory.php:27-28
The factory passes `$uri` to the `ServerRequest` constructor and then immediately calls `->withUri($uri)` with the exact same URI. The only effect of the second call is to populate the `Host` header, but it also creates an extra intermediate clone. Passing the `Host` header directly in the constructor avoids the redundant allocation.
```suggestion
$host = $uri->getHost() !== '' ? $uri->getHost() . ($uri->getPort() !== null ? ':' . $uri->getPort() : '') : null;
return new ServerRequest(
strtoupper($method),
$uri,
$serverParams,
headers: $host !== null ? ['Host' => $host] : [],
);
```
Reviews (1): Last reviewed commit: "Add PSR-7 server request implementation" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private static function isFileSpec(array $file): bool | ||
| { | ||
| return \array_key_exists('tmp_name', $file) | ||
| || \array_key_exists('error', $file) | ||
| || \array_key_exists('size', $file); | ||
| } |
There was a problem hiding this comment.
isFileSpec returns true when ANY of the three keys are present. This means an array like ['size' => 42, 'color' => 'blue'] would be treated as a file spec and passed to normalizeFileSpec, which would silently create new UploadedFile('', 42, 0, null, null) because tmp_name is absent. Using AND logic (requiring at least tmp_name + error) better matches the actual $_FILES shape and avoids creating bogus instances for incidentally matching arrays.
| private static function isFileSpec(array $file): bool | |
| { | |
| return \array_key_exists('tmp_name', $file) | |
| || \array_key_exists('error', $file) | |
| || \array_key_exists('size', $file); | |
| } | |
| private static function isFileSpec(array $file): bool | |
| { | |
| return \array_key_exists('tmp_name', $file) | |
| && \array_key_exists('error', $file); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/psr7/src/Psr7/UploadedFile.php
Line: 118-123
Comment:
`isFileSpec` returns `true` when ANY of the three keys are present. This means an array like `['size' => 42, 'color' => 'blue']` would be treated as a file spec and passed to `normalizeFileSpec`, which would silently create `new UploadedFile('', 42, 0, null, null)` because `tmp_name` is absent. Using AND logic (requiring at least `tmp_name` + `error`) better matches the actual `$_FILES` shape and avoids creating bogus instances for incidentally matching arrays.
```suggestion
private static function isFileSpec(array $file): bool
{
return \array_key_exists('tmp_name', $file)
&& \array_key_exists('error', $file);
}
```
How can I resolve this? If you propose a fix, please make it concise.
| foreach ($files as $key => $file) { | ||
| if (!\is_array($file)) { | ||
| continue; | ||
| } | ||
|
|
||
| $normalized[$key] = self::isFileSpec($file) |
There was a problem hiding this comment.
continue, so they are absent from the returned array entirely. If a caller passes an UploadedFileInterface instance that is already normalised (a reasonable thing to do when mixing normalised and raw $_FILES data), it is silently lost without any error. Passing it through to the result preserves the correct tree structure in those cases.
| foreach ($files as $key => $file) { | |
| if (!\is_array($file)) { | |
| continue; | |
| } | |
| $normalized[$key] = self::isFileSpec($file) | |
| foreach ($files as $key => $file) { | |
| if ($file instanceof UploadedFileInterface) { | |
| $normalized[$key] = $file; | |
| continue; | |
| } | |
| if (!\is_array($file)) { | |
| continue; | |
| } | |
| $normalized[$key] = self::isFileSpec($file) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/psr7/src/Psr7/UploadedFile.php
Line: 34-39
Comment:
Non-array entries are silently skipped with `continue`, so they are absent from the returned array entirely. If a caller passes an `UploadedFileInterface` instance that is already normalised (a reasonable thing to do when mixing normalised and raw `$_FILES` data), it is silently lost without any error. Passing it through to the result preserves the correct tree structure in those cases.
```suggestion
foreach ($files as $key => $file) {
if ($file instanceof UploadedFileInterface) {
$normalized[$key] = $file;
continue;
}
if (!\is_array($file)) {
continue;
}
$normalized[$key] = self::isFileSpec($file)
```
How can I resolve this? If you propose a fix, please make it concise.| return new ServerRequest(strtoupper($method), $uri, $serverParams) | ||
| ->withUri($uri); |
There was a problem hiding this comment.
$uri to the ServerRequest constructor and then immediately calls ->withUri($uri) with the exact same URI. The only effect of the second call is to populate the Host header, but it also creates an extra intermediate clone. Passing the Host header directly in the constructor avoids the redundant allocation.
| return new ServerRequest(strtoupper($method), $uri, $serverParams) | |
| ->withUri($uri); | |
| $host = $uri->getHost() !== '' ? $uri->getHost() . ($uri->getPort() !== null ? ':' . $uri->getPort() : '') : null; | |
| return new ServerRequest( | |
| strtoupper($method), | |
| $uri, | |
| $serverParams, | |
| headers: $host !== null ? ['Host' => $host] : [], | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/psr7/src/Psr7/ServerRequest/Factory.php
Line: 27-28
Comment:
The factory passes `$uri` to the `ServerRequest` constructor and then immediately calls `->withUri($uri)` with the exact same URI. The only effect of the second call is to populate the `Host` header, but it also creates an extra intermediate clone. Passing the `Host` header directly in the constructor avoids the redundant allocation.
```suggestion
$host = $uri->getHost() !== '' ? $uri->getHost() . ($uri->getPort() !== null ? ':' . $uri->getPort() : '') : null;
return new ServerRequest(
strtoupper($method),
$uri,
$serverParams,
headers: $host !== null ? ['Host' => $host] : [],
);
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
Utopia\Psr7\ServerRequestimplementing PSRServerRequestInterfaceRelease
utopia-php/psr7as0.2.0after this mergesVerification
bin/monorepo check psr7bin/monorepo test psr7