Update CI workflow for PHP versions #17
Conversation
Add PHP 8.3 to old_stable and update current_stable to PHP 8.4. This ensures compatibility checks with the latest PHP versions.
📝 Walkthrough📝 WalkthroughWalkthroughこのプルリクエストでは、 Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17 +/- ##
==============================================
- Coverage 100.00% 81.44% -18.56%
- Complexity 29 102 +73
==============================================
Files 5 8 +3
Lines 117 361 +244
==============================================
+ Hits 117 294 +177
- Misses 0 67 +67 ☔ View full report in Codecov by Sentry. |
Eliminated the 'twigPaths' parameter from the #[TwigPaths] attribute for cleaner code. This change simplifies the constructor definition and improves readability.
Bumped versions for several Composer dependencies including `composer/pcre`, `composer/semver`, and `symfony/config`. This update ensures compatibility with new features and fixes in these libraries, and adjusts `require-dev` settings and conflict resolutions accordingly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This update introduces a LogicException to handle cases where the resource body is not found in the provided HTML snippet. This ensures the code fails gracefully with a clear error message if the expected resource body is missing, improving the robustness of the rendering functionality.
Updated the plugin-api-version in two composer.lock files from 2.2.0 to 2.6.0. This also included changes in 'platform' and 'platform-dev' fields from empty arrays to empty objects.
The `twigPaths` array was commented out and is not used in the current implementation. Cleaning up this unnecessary code enhances readability and maintains a clean codebase. This change does not affect any functionality but results in a more maintainable code structure.
Upgraded the development dependency `bamarni/composer-bin-plugin` from version ^1.4 to ^1.8 in the `composer.json` file. This ensures compatibility with newer features and bug fixes provided in the latest version.
Upgraded PHPStan from version 1.12.11 to 2.0.2 in composer files. This change includes updates to version constraints and references in composer.lock and composer.json for better compatibility with PHP versions 7.4 and above.
Added missing type annotations for better static analysis and clarified code behavior with comments. Removed an unnecessary function call and added a null-check to ensure stability during header parsing.
Introduce try-catch block in __invoke to handle exceptions and return a NullResourceObject with error details. Extract invocation logic into a new invoke method and improve getCode to handle invalid status strings gracefully.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
vendor-bin/tools/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
composer.json(1 hunks)src/DevInvoker.php(2 hunks)src/Halo/TemplateLocator.php(1 hunks)src/Http/CreateResponse.php(1 hunks)src/Http/HttpResource.php(1 hunks)vendor-bin/tools/composer.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Halo/TemplateLocator.php
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/Http/CreateResponse.php
[warning] 36-36: src/Http/CreateResponse.php#L36
Added line #L36 was not covered by tests
src/DevInvoker.php
[warning] 82-82: src/DevInvoker.php#L82
Added line #L82 was not covered by tests
🪛 GitHub Actions: Continuous Integration
src/Http/CreateResponse.php
[error] 61-61: AssertionError: assert(array_key_exists(0, $match)) failed
🪛 GitHub Check: sa / Psalm
src/DevInvoker.php
[failure] 82-82:
InvalidPropertyAssignmentValue: $resource->headers with declared type 'array<string, string>' cannot be assigned type 'non-empty-array<string, array<string, string>|string>'
🔇 Additional comments (5)
vendor-bin/tools/composer.json (1)
6-6: PHPStan のメジャーバージョンアップデートの確認が必要ですPHPStan を v1.8 から v2.0 へメジャーバージョンアップする変更が含まれています。以下の点について確認をお願いします：
- 破壊的変更の有無
- PHP 8.3/8.4 との互換性
- 既存の PHPStan 設定ファイルの互換性
✅ Verification successful
PHPStan v2.0 へのアップグレードは安全に実施可能です
プロジェクトの構成を確認した結果、以下の点から PHPStan v2.0 への移行は問題ないと判断できます：
- プロジェクトの PHPStan 設定（
phpstan.neon）はシンプルで、特別なカスタマイズがないため、互換性の問題は発生しにくい状態です- メインの
composer.jsonで PHP 8.0 以上をサポートしており、PHPStan v2.0 の要件と互換性があります- CI スクリプトで使用されている
phpstan analyseコマンドの構文は v2.0 でも変更ありません🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # PHPStan の設定ファイルを検索 rg -l 'phpstan' -g '*.neon'Length of output: 40
Script:
#!/bin/bash # PHPStan の設定ファイルの内容を確認 cat phpstan.neonLength of output: 97
Script:
#!/bin/bash # composer.json の内容を確認して PHP バージョン要件を確認 cat vendor-bin/tools/composer.jsonLength of output: 503
Script:
#!/bin/bash # プロジェクトのメインの composer.json も確認 cat composer.jsonLength of output: 2994
composer.json (2)
25-25: composer-bin-plugin のバージョン更新について
bamarni/composer-bin-pluginを v1.4 から v1.8 へアップデートする変更は適切です。このアップデートは：
- 同じメジャーバージョン内での更新なので、後方互換性は維持されています
- PHP 8.3/8.4 のサポートを含む可能性が高いです
✅ Verification successful
composer-bin-plugin のバージョン更新は安全です
composer.json の確認結果から：
- プロジェクトは PHP 8.0 以上をサポート
- bamarni/composer-bin-plugin v1.8 は開発依存関係として適切に設定
- プラグインは composer.json の設定で正しく有効化
v1.4 から v1.8 へのマイナーバージョンアップは、後方互換性が保証されており、PHP 8.0 以上との互換性も維持されているため、安全なアップデートと判断できます。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # composer.json ファイル内の PHP バージョン要件を確認 rg '"php":\s*".*"' composer.json # プラグインの設定を確認 rg 'bamarni/composer-bin-plugin' -A 5 composer.jsonLength of output: 441
Line range hint
11-21: 依存パッケージの PHP 8.3/8.4 互換性の確認以下のパッケージについて、PHP 8.3/8.4 との互換性を確認することを推奨します：
aura/sql:^3 || ^4 || ^5symfony/process:^v5.4 || ^v6.4 || ^v7.0- その他の主要な依存パッケージ
src/Http/CreateResponse.php (1)
35-37: テストカバレッジを追加し、$line === nullのケースを検証してください。
$lineがnullの場合にループを終了させるロジック自体は妥当ですが、テストでこのケースをカバーしないと、実行時の意図しない動作に気づけない可能性があります。この分岐をテストするシナリオを作成できます。ご希望であれば、新たなユニットテスト例を提案しましょうか？
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 36-36: src/Http/CreateResponse.php#L36
Added line #L36 was not covered by testssrc/Http/HttpResource.php (1)
198-198: 型指定の強化は適切です。
$queryのキーを文字列に限定することで、可読性と安全性が向上します。現状の実装では懸念はありません。
| /** @var array<string, string> $profileId */ | ||
| $resource->headers[self::HEADER_PROFILE_ID] = $profileId; // @phpstan-ignore-line |
There was a problem hiding this comment.
$resource->headersへの代入時の型の不整合を修正してください。
$resource->headers は array<string, string> と定義されていますが、$profileId が配列であるため、直接代入すると型エラーになります。代わりにJSON文字列として格納する等の修正が必要です。
以下の差分例では json_encode() で配列を文字列化してヘッダーに設定する案を示しています。
- /** @var array<string, string> $profileId */
- $resource->headers[self::HEADER_PROFILE_ID] = $profileId; // @phpstan-ignore-line
+ $resource->headers[self::HEADER_PROFILE_ID] = json_encode($profileId, JSON_THROW_ON_ERROR);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** @var array<string, string> $profileId */ | |
| $resource->headers[self::HEADER_PROFILE_ID] = $profileId; // @phpstan-ignore-line | |
| $resource->headers[self::HEADER_PROFILE_ID] = json_encode($profileId, JSON_THROW_ON_ERROR); |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 82-82: src/DevInvoker.php#L82
Added line #L82 was not covered by tests
🪛 GitHub Check: sa / Psalm
[failure] 82-82:
InvalidPropertyAssignmentValue: $resource->headers with declared type 'array<string, string>' cannot be assigned type 'non-empty-array<string, array<string, string>|string>'
Replaced backslashes with forward slashes in class paths to ensure compatibility across Windows and other platforms. This change affects both weaved and non-weaved resource objects.
Added a `var_dump` to log URI and output data during response creation. Enhanced status parsing with detailed `error_log` messages to aid in debugging missing or malformed status codes.
Added a `var_dump` to log URI and output data during response creation. Enhanced status parsing with detailed `error_log` messages to aid in debugging missing or malformed status codes.
Reorganized header and body processing for clarity and removed unused or redundant code such as `var_dump` and `array_pop`. Improved error handling and ensured consistent parsing of headers. Simplified status code extraction and response body management by leveraging better practices.
Replaced exec with proc_open to improve process control and output handling for curl commands. Added fallback to exec for compatibility with Windows systems. This change ensures better error handling and resource management during HTTP requests.
Added a temporary var_dump to debug the exec output in the HTTP resource handling. This will assist in diagnosing potential issues in curl execution results. Remember to remove it in production-ready code.
Changed the regex in CreateResponse to ensure it matches only status lines starting at the beginning of the string. This prevents potential misinterpretation of malformed input and improves overall response handling accuracy.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/Halo/TemplateLocator.php (1)
70-74: パスの変換処理の一貫性を改善Windowsパスの変換処理が戻り値の部分でのみ行われています。一貫性のある処理にするため、メソッドの先頭で変換することを推奨します。
以下のリファクタリングを提案します：
private function getClass(ResourceObject $ro): string { + $className = get_class($ro); + $normalizedClassName = str_replace('\\', '/', $className); + if ($ro instanceof WeavedInterface) { $parentClass = (new ReflectionClass($ro))->getParentClass(); if (! $parentClass) { throw new \RuntimeException('Parent class not found'); } return str_replace('\\', '/', $parentClass->name); } - return str_replace('\\', '/', get_class($ro)); + return $normalizedClassName; }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-71: src/Halo/TemplateLocator.php#L71
Added line #L71 was not covered by tests
[warning] 74-74: src/Halo/TemplateLocator.php#L74
Added line #L74 was not covered by tests🪛 GitHub Check: sa / Psalm
[failure] 71-71:
PossiblyInvalidPropertyFetch: Cannot fetch property on possible non-object $parentClass of type false🪛 GitHub Check: sa / PHPStan
[failure] 71-71:
Cannot access property $name on Ray\Aop\ReflectionClass|false.🪛 phpstan (2.0.3)
71-71: Cannot access property $name on Ray\Aop\ReflectionClass|false.
(property.nonObject)
src/Http/HttpResource.php (1)
256-259: Windows対応のドキュメント化Windows環境での
proc_openのフォールバック処理が追加されていますが、ドキュメントが必要です。以下のようなドキュメントの追加を提案します：
+ /** + * Windows環境では proc_open が正常に動作しない場合があるため、 + * その場合は exec にフォールバックします。 + */ if (empty($output)) { // Windows対応のためexecを使用 exec($curl, $output); }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 258-258: src/Http/HttpResource.php#L258
Added line #L258 was not covered by tests🪛 GitHub Check: cs / Coding Standards
[failure] 256-256:
Expected 1 line after "if", found 0.📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro📒 Files selected for processing (3)
src/Halo/TemplateLocator.php(2 hunks)src/Http/CreateResponse.php(4 hunks)src/Http/HttpResource.php(3 hunks)🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/Http/CreateResponse.php
[warning] 27-31: src/Http/CreateResponse.php#L27-L31
Added lines #L27 - L31 were not covered by tests
[warning] 33-33: src/Http/CreateResponse.php#L33
Added line #L33 was not covered by tests
[warning] 71-71: src/Http/CreateResponse.php#L71
Added line #L71 was not covered by tests
[warning] 78-78: src/Http/CreateResponse.php#L78
Added line #L78 was not covered by tests
[warning] 91-91: src/Http/CreateResponse.php#L91
Added line #L91 was not covered by testssrc/Http/HttpResource.php
[warning] 239-239: src/Http/HttpResource.php#L239
Added line #L239 was not covered by tests
[warning] 258-258: src/Http/HttpResource.php#L258
Added line #L258 was not covered by testssrc/Halo/TemplateLocator.php
[warning] 71-71: src/Halo/TemplateLocator.php#L71
Added line #L71 was not covered by tests
[warning] 74-74: src/Halo/TemplateLocator.php#L74
Added line #L74 was not covered by tests🪛 GitHub Check: sa / PHPStan
src/Http/CreateResponse.php
[failure] 38-38:
Cannot cast mixed to string.
[failure] 58-58:
Parameter #1 $headers of method BEAR\Dev\Http\CreateResponse::getHeaders() expects array, list given.
[failure] 61-61:
Parameter #1 $body of method BEAR\Dev\Http\CreateResponse::getJsonView() expects array, list given.
[failure] 105-105:
Parameter #2 $callback of function array_filter expects (callable(string): bool)|null, 'strlen' given.src/Http/HttpResource.php
[failure] 243-243:
Cannot access offset 1 on mixed.src/Halo/TemplateLocator.php
[failure] 71-71:
Cannot access property $name on Ray\Aop\ReflectionClass|false.🪛 phpstan (2.0.3)
src/Http/CreateResponse.php
38-38: Cannot cast mixed to string.
(cast.string)
58-58: Parameter #1 $headers of method BEAR\Dev\Http\CreateResponse::getHeaders() expects array, list given.
(argument.type)
61-61: Parameter #1 $body of method BEAR\Dev\Http\CreateResponse::getJsonView() expects array, list given.
(argument.type)
105-105: Parameter #2 $callback of function array_filter expects (callable(string): bool)|null, 'strlen' given.
(argument.type)
src/Http/HttpResource.php
243-243: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
243-243: Parameter #1 $stream of function feof expects resource, mixed given.
(argument.type)
244-244: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
244-244: Parameter #1 $stream of function fgets expects resource, mixed given.
(argument.type)
252-252: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
252-252: Parameter #1 $stream of function fclose expects resource, mixed given.
(argument.type)
253-253: Cannot access offset 2 on mixed.
(offsetAccess.nonOffsetAccessible)
253-253: Parameter #1 $stream of function fclose expects resource, mixed given.
(argument.type)
src/Halo/TemplateLocator.php
71-71: Cannot access property $name on Ray\Aop\ReflectionClass|false.
(property.nonObject)
🪛 GitHub Actions: Static Analysis
src/Http/CreateResponse.php
[warning] 50-50: The method __invoke uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.
🪛 GitHub Check: cs / Coding Standards
src/Http/HttpResource.php
[failure] 30-30:
Header blocks must not contain blank lines
[failure] 32-32:
Expected 0 lines between same types of use statement, found 1.
[failure] 32-32:
Header blocks must be separated by a single blank line
[failure] 256-256:
Expected 1 line after "if", found 0.🪛 GitHub Check: sa / Psalm
src/Http/HttpResource.php
[failure] 260-260:
ForbiddenCode: Unsafe var_dumpsrc/Halo/TemplateLocator.php
[failure] 71-71:
PossiblyInvalidPropertyFetch: Cannot fetch property on possible non-object $parentClass of type false🔇 Additional comments (2)
src/Halo/TemplateLocator.php (1)
70-74: テストカバレッジの追加が必要です新しく追加されたWindowsパス対応のコードに対するテストが不足しています。
以下のシナリオのテストケース追加を推奨します：
- 通常のクラス名の変換
- WeavedInterfaceを実装したクラスの変換
- 親クラスが存在しない場合の例外処理
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-71: src/Halo/TemplateLocator.php#L71
Added line #L71 was not covered by tests
[warning] 74-74: src/Halo/TemplateLocator.php#L74
Added line #L74 was not covered by tests🪛 GitHub Check: sa / Psalm
[failure] 71-71:
PossiblyInvalidPropertyFetch: Cannot fetch property on possible non-object $parentClass of type false🪛 GitHub Check: sa / PHPStan
[failure] 71-71:
Cannot access property $name on Ray\Aop\ReflectionClass|false.🪛 phpstan (2.0.3)
71-71: Cannot access property $name on Ray\Aop\ReflectionClass|false.
(property.nonObject)
src/Http/CreateResponse.php (1)
26-34: 空のレスポンス処理の追加は良い改善です空のレスポンスに対する適切なエラーハンドリングが追加されていますが、テストカバレッジが必要です。
以下のテストケースの追加を推奨します：
- 空の出力配列を渡した場合のエラーレスポンス
- エラーメッセージの内容確認
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 27-31: src/Http/CreateResponse.php#L27-L31
Added lines #L27 - L31 were not covered by tests
[warning] 33-33: src/Http/CreateResponse.php#L33
Added line #L33 was not covered by tests
| // Convert for Windows support | ||
| return str_replace('\\', '/', $parentClass->name); |
There was a problem hiding this comment.
親クラスの存在チェックが必要です
getParentClass()はfalseを返す可能性があるため、nullチェックを追加する必要があります。
以下の修正を提案します：
- return str_replace('\\', '/', $parentClass->name);
+ if (! $parentClass) {
+ throw new \RuntimeException('Parent class not found');
+ }
+ return str_replace('\\', '/', $parentClass->name);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Convert for Windows support | |
| return str_replace('\\', '/', $parentClass->name); | |
| // Convert for Windows support | |
| if (! $parentClass) { | |
| throw new \RuntimeException('Parent class not found'); | |
| } | |
| return str_replace('\\', '/', $parentClass->name); |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-71: src/Halo/TemplateLocator.php#L71
Added line #L71 was not covered by tests
🪛 GitHub Check: sa / Psalm
[failure] 71-71:
PossiblyInvalidPropertyFetch: Cannot fetch property on possible non-object $parentClass of type false
🪛 GitHub Check: sa / PHPStan
[failure] 71-71:
Cannot access property $name on Ray\Aop\ReflectionClass|false.
🪛 phpstan (2.0.3)
71-71: Cannot access property $name on Ray\Aop\ReflectionClass|false.
(property.nonObject)
| // ヘッダー部分の処理 | ||
| $headerComplete = false; | ||
| foreach ($output as $line) { | ||
| if (! $headerComplete) { | ||
| if ($line === '') { | ||
| $headerComplete = true; | ||
| continue; | ||
| } | ||
|
|
||
| $headers[] = $line; | ||
| } else { | ||
| $body[] = $line; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ヘッダー処理のリファクタリングについて
ヘッダー処理のロジックが改善されていますが、型の安全性に関する問題があります。
以下の修正を提案します：
- foreach ($output as $line) {
+ foreach ($output as $line) {
+ if (! is_string($line)) {
+ continue;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ヘッダー部分の処理 | |
| $headerComplete = false; | |
| foreach ($output as $line) { | |
| if (! $headerComplete) { | |
| if ($line === '') { | |
| $headerComplete = true; | |
| continue; | |
| } | |
| $headers[] = $line; | |
| } else { | |
| $body[] = $line; | |
| } | |
| } | |
| // ヘッダー部分の処理 | |
| $headerComplete = false; | |
| foreach ($output as $line) { | |
| if (! is_string($line)) { | |
| continue; | |
| } | |
| if (! $headerComplete) { | |
| if ($line === '') { | |
| $headerComplete = true; | |
| continue; | |
| } | |
| $headers[] = $line; | |
| } else { | |
| $body[] = $line; | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Static Analysis
[warning] 50-50: The method __invoke uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.
| @@ -85,7 +102,7 @@ | |||
| */ | |||
| private function getJsonView(array $body): string | |||
| { | |||
| array_pop($body); | |||
| $body = array_filter($body, 'strlen'); | |||
There was a problem hiding this comment.
array_filterの使用方法の改善
array_filterの使用方法に型の安全性の問題があります。
以下の修正を提案します：
- $body = array_filter($body, 'strlen');
+ $body = array_filter($body, static function (string $line): bool {
+ return $line !== '';
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $body = array_filter($body, 'strlen'); | |
| $body = array_filter($body, static function (string $line): bool { | |
| return $line !== ''; | |
| }); |
🧰 Tools
🪛 GitHub Check: sa / PHPStan
[failure] 105-105:
Parameter #2 $callback of function array_filter expects (callable(string): bool)|null, 'strlen' given.
🪛 phpstan (2.0.3)
105-105: Parameter #2 $callback of function array_filter expects (callable(string): bool)|null, 'strlen' given.
(argument.type)
src/Http/HttpResource.php
Outdated
| // Windows対応のためexecを使用 | ||
| exec($curl, $output); | ||
| } | ||
| var_dump($output); |
There was a problem hiding this comment.
デバッグコードの削除
本番環境でvar_dumpを使用することは避けるべきです。
以下の修正を提案します：
- var_dump($output);🧰 Tools
🪛 GitHub Check: sa / Psalm
[failure] 260-260:
ForbiddenCode: Unsafe var_dump
src/Http/HttpResource.php
Outdated
| $descriptorspec = [ | ||
| 1 => ['pipe', 'w'], // stdout | ||
| 2 => ['pipe', 'w'], // stderr | ||
| ]; | ||
|
|
||
| $process = proc_open($curl, $descriptorspec, $pipes); | ||
| if (! is_resource($process)) { | ||
| throw new RuntimeException('Failed to execute curl command'); | ||
| } | ||
|
|
||
| $output = []; | ||
| while (! feof($pipes[1])) { | ||
| $line = fgets($pipes[1]); | ||
| if ($line === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $output[] = trim($line); | ||
| } | ||
|
|
||
| fclose($pipes[1]); | ||
| fclose($pipes[2]); | ||
| proc_close($process); | ||
|
|
There was a problem hiding this comment.
プロセス処理の改善が必要です
プロセスとリソースの処理に型の安全性の問題があります。
以下の修正を提案します：
$process = proc_open($curl, $descriptorspec, $pipes);
if (! is_resource($process)) {
throw new RuntimeException('Failed to execute curl command');
}
+if (! isset($pipes[1]) || ! is_resource($pipes[1]) || ! isset($pipes[2]) || ! is_resource($pipes[2])) {
+ throw new RuntimeException('Failed to open pipes');
+}
$output = [];
-while (! feof($pipes[1])) {
+while ($pipes[1] && ! feof($pipes[1])) {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 239-239: src/Http/HttpResource.php#L239
Added line #L239 was not covered by tests
🪛 GitHub Check: sa / PHPStan
[failure] 243-243:
Cannot access offset 1 on mixed.
🪛 phpstan (2.0.3)
243-243: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
243-243: Parameter #1 $stream of function feof expects resource, mixed given.
(argument.type)
244-244: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
244-244: Parameter #1 $stream of function fgets expects resource, mixed given.
(argument.type)
252-252: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
252-252: Parameter #1 $stream of function fclose expects resource, mixed given.
(argument.type)
253-253: Cannot access offset 2 on mixed.
(offsetAccess.nonOffsetAccessible)
253-253: Parameter #1 $stream of function fclose expects resource, mixed given.
(argument.type)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/Http/CreateResponse.php (2)
26-34: 空のレスポンス処理の追加について空のレスポンスに対する処理が追加されましたが、テストカバレッジがありません。また、エラーメッセージの国際化も検討する必要があります。
以下の改善を提案します：
- $ro->body = ['error' => 'Empty response']; + $ro->body = ['error' => ['message' => 'Empty response', 'code' => 'EMPTY_RESPONSE']];🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 27-31: src/Http/CreateResponse.php#L27-L31
Added lines #L27 - L31 were not covered by tests
[warning] 33-33: src/Http/CreateResponse.php#L33
Added line #L33 was not covered by tests
40-53: ヘッダー処理のリファクタリング提案現在の実装は
else句を使用していますが、早期リターンパターンを使用することで、コードの可読性が向上します。- if (! $headerComplete) { - if ($line === '') { - $headerComplete = true; - continue; - } - $headers[] = $line; - } else { - $body[] = $line; - } + if (! $headerComplete && $line === '') { + $headerComplete = true; + continue; + } + + if (! $headerComplete) { + $headers[] = $line; + continue; + } + + $body[] = $line;🧰 Tools
🪛 GitHub Actions: Static Analysis
[warning] 50-50: The method __invoke uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Halo/TemplateLocator.php(2 hunks)src/Http/CreateResponse.php(4 hunks)src/Http/HttpResource.php(3 hunks)
🧰 Additional context used
🪛 GitHub Check: cs / Coding Standards
src/Http/HttpResource.php
[failure] 30-30:
Header blocks must not contain blank lines
[failure] 32-32:
Expected 0 lines between same types of use statement, found 1.
[failure] 32-32:
Header blocks must be separated by a single blank line
[failure] 256-256:
Expected 1 line after "if", found 0.
🪛 GitHub Check: codecov/patch
src/Http/HttpResource.php
[warning] 239-239: src/Http/HttpResource.php#L239
Added line #L239 was not covered by tests
[warning] 258-258: src/Http/HttpResource.php#L258
Added line #L258 was not covered by tests
src/Halo/TemplateLocator.php
[warning] 71-71: src/Halo/TemplateLocator.php#L71
Added line #L71 was not covered by tests
[warning] 74-74: src/Halo/TemplateLocator.php#L74
Added line #L74 was not covered by tests
src/Http/CreateResponse.php
[warning] 27-31: src/Http/CreateResponse.php#L27-L31
Added lines #L27 - L31 were not covered by tests
[warning] 33-33: src/Http/CreateResponse.php#L33
Added line #L33 was not covered by tests
[warning] 71-71: src/Http/CreateResponse.php#L71
Added line #L71 was not covered by tests
[warning] 78-78: src/Http/CreateResponse.php#L78
Added line #L78 was not covered by tests
[warning] 91-91: src/Http/CreateResponse.php#L91
Added line #L91 was not covered by tests
🪛 GitHub Check: sa / Psalm
src/Http/HttpResource.php
[failure] 260-260:
ForbiddenCode: Unsafe var_dump
src/Halo/TemplateLocator.php
[failure] 71-71:
PossiblyInvalidPropertyFetch: Cannot fetch property on possible non-object $parentClass of type false
🪛 GitHub Check: sa / PHPStan
src/Http/HttpResource.php
[failure] 243-243:
Cannot access offset 1 on mixed.
src/Halo/TemplateLocator.php
[failure] 71-71:
Cannot access property $name on Ray\Aop\ReflectionClass|false.
src/Http/CreateResponse.php
[failure] 38-38:
Cannot cast mixed to string.
[failure] 58-58:
Parameter #1 $headers of method BEAR\Dev\Http\CreateResponse::getHeaders() expects array, list given.
[failure] 61-61:
Parameter #1 $body of method BEAR\Dev\Http\CreateResponse::getJsonView() expects array, list given.
[failure] 105-105:
Parameter #2 $callback of function array_filter expects (callable(string): bool)|null, 'strlen' given.
🪛 phpstan (2.0.3)
src/Http/HttpResource.php
243-243: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
243-243: Parameter #1 $stream of function feof expects resource, mixed given.
(argument.type)
244-244: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
244-244: Parameter #1 $stream of function fgets expects resource, mixed given.
(argument.type)
252-252: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
252-252: Parameter #1 $stream of function fclose expects resource, mixed given.
(argument.type)
253-253: Cannot access offset 2 on mixed.
(offsetAccess.nonOffsetAccessible)
253-253: Parameter #1 $stream of function fclose expects resource, mixed given.
(argument.type)
src/Halo/TemplateLocator.php
71-71: Cannot access property $name on Ray\Aop\ReflectionClass|false.
(property.nonObject)
src/Http/CreateResponse.php
38-38: Cannot cast mixed to string.
(cast.string)
58-58: Parameter #1 $headers of method BEAR\Dev\Http\CreateResponse::getHeaders() expects array, list given.
(argument.type)
61-61: Parameter #1 $body of method BEAR\Dev\Http\CreateResponse::getJsonView() expects array, list given.
(argument.type)
105-105: Parameter #2 $callback of function array_filter expects (callable(string): bool)|null, 'strlen' given.
(argument.type)
🪛 GitHub Actions: Static Analysis
src/Http/CreateResponse.php
[warning] 50-50: The method __invoke uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.
🔇 Additional comments (1)
src/Halo/TemplateLocator.php (1)
29-29: アノテーションの変更を確認してください
#[TwigPaths]アノテーションの引数が削除されましたが、この変更が意図的なものか確認してください。✅ Verification successful
#[TwigPaths]アノテーションの引数なしの使用は問題ありませんコードベース全体を確認したところ、
#[TwigPaths]アノテーションは引数なしで使用されており、これが現在の正しい使用方法であることが確認できました。他のファイルでも同様のパターンが見られ、一貫性が保たれています。🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify TwigPaths annotation usage across the codebase ast-grep --pattern 'class $_ { $$$ #[TwigPaths($_)] private $_ $_ = $_ $$$ }'Length of output: 85
Script:
#!/bin/bash # Search for TwigPaths annotation usage in the codebase rg -A 2 "#\[TwigPaths"Length of output: 286
Script:
#!/bin/bash # Search for files containing TwigPaths class or interface definition rg "class.*TwigPaths|interface.*TwigPaths"Length of output: 44
Script:
#!/bin/bash # Find and show content of files containing TwigPaths in their name fd -t f "TwigPaths" -x cat {}Length of output: 31
Script:
#!/bin/bash # Search for TwigPaths attribute/annotation definition rg -A 5 "class.*TwigPaths|attribute.*TwigPaths" # Also search in PHP files specifically fd -e php -x cat {} | rg -A 5 "class.*TwigPaths|attribute.*TwigPaths"Length of output: 123
| // Convert for Windows support | ||
| return str_replace('\\', '/', $parentClass->name); | ||
| } | ||
|
|
||
| return get_class($ro); | ||
| return str_replace('\\', '/', get_class($ro)); |
There was a problem hiding this comment.
💡 Codebase verification
getParentClass()の戻り値チェックとテストカバレッジの問題について
コードベースの分析結果から以下の点が確認できました：
HaloRendererクラスではgetParentClass()の戻り値チェックが実装されていますが、TemplateLocatorでは実装されていません。LogicExceptionの使用パターンが他のクラスでも確認できます。- Windowsパスに関連するテストが見つかりませんでした。
修正提案：
getParentClass()の戻り値チェックを追加
if ($parentClass === false) {
throw new LogicException('Failed to get parent class');
}- Windowsパス変換のテストケースを追加することを推奨します。
🔗 Analysis chain
Windows対応の実装に関する重要な問題
getParentClass()の戻り値がfalseの場合のハンドリングが必要です。- 未テストのコードパスが存在します。
以下の修正を提案します：
- return str_replace('\\', '/', $parentClass->name);
+ $parentClass = (new ReflectionClass($ro))->getParentClass();
+ if ($parentClass === false) {
+ throw new LogicException('Failed to get parent class');
+ }
+ return str_replace('\\', '/', $parentClass->name);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for Windows path handling
rg -l "test.*getClass.*Windows"
Length of output: 33
Script:
#!/bin/bash
# Check for test files related to TemplateLocator
fd "TemplateLocator.*Test\.php"
# Search for Windows path handling tests in general
rg -l "test.*[Ww]indows.*path"
# Check implementation of getParentClass handling
ast-grep --pattern 'getParentClass()'
Length of output: 106
Script:
#!/bin/bash
# Check current implementation around the code in question
rg -A 5 -B 5 "getParentClass\(\)" src/
Length of output: 1510
Script:
#!/bin/bash
# Look for existing exception handling patterns
rg "LogicException|RuntimeException" src/
Length of output: 777
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-71: src/Halo/TemplateLocator.php#L71
Added line #L71 was not covered by tests
[warning] 74-74: src/Halo/TemplateLocator.php#L74
Added line #L74 was not covered by tests
🪛 GitHub Check: sa / Psalm
[failure] 71-71:
PossiblyInvalidPropertyFetch: Cannot fetch property on possible non-object $parentClass of type false
🪛 GitHub Check: sa / PHPStan
[failure] 71-71:
Cannot access property $name on Ray\Aop\ReflectionClass|false.
🪛 phpstan (2.0.3)
71-71: Cannot access property $name on Ray\Aop\ReflectionClass|false.
(property.nonObject)
| @@ -85,7 +102,7 @@ | |||
| */ | |||
| private function getJsonView(array $body): string | |||
| { | |||
| array_pop($body); | |||
| $body = array_filter($body, 'strlen'); | |||
There was a problem hiding this comment.
型の安全性に関する問題
array_filterの使用方法に問題があります。コールバック関数の型が正しく指定されていません。
- $body = array_filter($body, 'strlen');
+ $body = array_filter($body, static fn(string $line): bool => $line !== '');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $body = array_filter($body, 'strlen'); | |
| $body = array_filter($body, static fn(string $line): bool => $line !== ''); |
🧰 Tools
🪛 GitHub Check: sa / PHPStan
[failure] 105-105:
Parameter #2 $callback of function array_filter expects (callable(string): bool)|null, 'strlen' given.
🪛 phpstan (2.0.3)
105-105: Parameter #2 $callback of function array_filter expects (callable(string): bool)|null, 'strlen' given.
(argument.type)
src/Http/HttpResource.php
Outdated
| if (empty($output)) { | ||
| // Windows対応のためexecを使用 | ||
| exec($curl, $output); | ||
| } | ||
| var_dump($output); |
There was a problem hiding this comment.
デバッグコードの削除とWindows対応の改善
var_dumpが本番コードに残されています- Windows対応のコメントが日本語で書かれています
- 実行結果の検証が不足しています
if (empty($output)) {
- // Windows対応のためexecを使用
+ // Fallback to exec for Windows compatibility
exec($curl, $output);
+ if (empty($output)) {
+ throw new RuntimeException('Failed to execute curl command: no output');
+ }
}
-var_dump($output);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (empty($output)) { | |
| // Windows対応のためexecを使用 | |
| exec($curl, $output); | |
| } | |
| var_dump($output); | |
| if (empty($output)) { | |
| // Fallback to exec for Windows compatibility | |
| exec($curl, $output); | |
| if (empty($output)) { | |
| throw new RuntimeException('Failed to execute curl command: no output'); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 258-258: src/Http/HttpResource.php#L258
Added line #L258 was not covered by tests
🪛 GitHub Check: sa / Psalm
[failure] 260-260:
ForbiddenCode: Unsafe var_dump
🪛 GitHub Check: cs / Coding Standards
[failure] 256-256:
Expected 1 line after "if", found 0.
src/Http/HttpResource.php
Outdated
| $descriptorspec = [ | ||
| 1 => ['pipe', 'w'], // stdout | ||
| 2 => ['pipe', 'w'], // stderr | ||
| ]; | ||
|
|
||
| $process = proc_open($curl, $descriptorspec, $pipes); | ||
| if (! is_resource($process)) { | ||
| throw new RuntimeException('Failed to execute curl command'); | ||
| } | ||
|
|
||
| $output = []; | ||
| while (! feof($pipes[1])) { | ||
| $line = fgets($pipes[1]); | ||
| if ($line === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $output[] = trim($line); | ||
| } | ||
|
|
||
| fclose($pipes[1]); | ||
| fclose($pipes[2]); | ||
| proc_close($process); | ||
|
|
There was a problem hiding this comment.
プロセス処理の改善が必要
- リソースの型安全性が確保されていません
- エラー出力（stderr）の処理が不足しています
以下の改善を提案します：
$descriptorspec = [
1 => ['pipe', 'w'], // stdout
2 => ['pipe', 'w'], // stderr
];
$process = proc_open($curl, $descriptorspec, $pipes);
if (! is_resource($process)) {
throw new RuntimeException('Failed to execute curl command');
}
+/** @var resource $stdout */
+$stdout = $pipes[1];
+/** @var resource $stderr */
+$stderr = $pipes[2];
+
$output = [];
-while (! feof($pipes[1])) {
- $line = fgets($pipes[1]);
+while (! feof($stdout)) {
+ $line = fgets($stdout);
if ($line === false) {
continue;
}
$output[] = trim($line);
}
-fclose($pipes[1]);
-fclose($pipes[2]);
+fclose($stdout);
+fclose($stderr);
proc_close($process);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $descriptorspec = [ | |
| 1 => ['pipe', 'w'], // stdout | |
| 2 => ['pipe', 'w'], // stderr | |
| ]; | |
| $process = proc_open($curl, $descriptorspec, $pipes); | |
| if (! is_resource($process)) { | |
| throw new RuntimeException('Failed to execute curl command'); | |
| } | |
| $output = []; | |
| while (! feof($pipes[1])) { | |
| $line = fgets($pipes[1]); | |
| if ($line === false) { | |
| continue; | |
| } | |
| $output[] = trim($line); | |
| } | |
| fclose($pipes[1]); | |
| fclose($pipes[2]); | |
| proc_close($process); | |
| $descriptorspec = [ | |
| 1 => ['pipe', 'w'], // stdout | |
| 2 => ['pipe', 'w'], // stderr | |
| ]; | |
| $process = proc_open($curl, $descriptorspec, $pipes); | |
| if (! is_resource($process)) { | |
| throw new RuntimeException('Failed to execute curl command'); | |
| } | |
| /** @var resource $stdout */ | |
| $stdout = $pipes[1]; | |
| /** @var resource $stderr */ | |
| $stderr = $pipes[2]; | |
| $output = []; | |
| while (! feof($stdout)) { | |
| $line = fgets($stdout); | |
| if ($line === false) { | |
| continue; | |
| } | |
| $output[] = trim($line); | |
| } | |
| fclose($stdout); | |
| fclose($stderr); | |
| proc_close($process); |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 239-239: src/Http/HttpResource.php#L239
Added line #L239 was not covered by tests
🪛 GitHub Check: sa / PHPStan
[failure] 243-243:
Cannot access offset 1 on mixed.
🪛 phpstan (2.0.3)
243-243: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
243-243: Parameter #1 $stream of function feof expects resource, mixed given.
(argument.type)
244-244: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
244-244: Parameter #1 $stream of function fgets expects resource, mixed given.
(argument.type)
252-252: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
252-252: Parameter #1 $stream of function fclose expects resource, mixed given.
(argument.type)
253-253: Cannot access offset 2 on mixed.
(offsetAccess.nonOffsetAccessible)
253-253: Parameter #1 $stream of function fclose expects resource, mixed given.
(argument.type)
Changed the regex in CreateResponse to ensure it matches only status lines starting at the beginning of the string. This prevents potential misinterpretation of malformed input and improves overall response handling accuracy.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/Fake/app/src/Injector.php (2)
11-12: 組み込み関数の use について
str_replace と var_dump を明示的に use することは、可読性を高める一方で、名前空間の競合がほぼ無い関数については省略も検討できます。チームのコーディング規約に合わせて決めると良いでしょう。
29-31: デバッグ出力の使用に注意
var_dumpを本番環境で使用すると、予期せぬ機密情報が出力される恐れがあります。必要に応じてログ機構や条件分岐を導入し、安全なデバッグ手法を検討してください。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/Fake/app/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
tests/Fake/app/src/Injector.php(2 hunks)
🔇 Additional comments (1)
tests/Fake/app/src/Injector.php (1)
7-7: Metaクラスの新規インポートが適切です
Meta クラスの導入によってアプリ固有のメタデータを管理できるようになり、コードの可読性・保守性が向上しています。問題ありません。
Changed the regex in CreateResponse to ensure it matches only status lines starting at the beginning of the string. This prevents potential misinterpretation of malformed input and improves overall response handling accuracy.
Switched the bear/package dependency to "dev-windows as 1.18" in require-dev. This ensures compatibility with Windows-specific development requirements while maintaining version constraints.
Abstracted request execution into dedicated methods for better readability and added fallback logic for `proc_open` with `exec`. Enhanced security by escaping JSON payloads in curl commands and replaced inconsistent URI construction with a consistent approach.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
composer.json(1 hunks)src/Http/HttpResource.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- composer.json
🧰 Additional context used
🪛 phpstan (2.0.3)
src/Http/HttpResource.php
90-90: Method BEAR\Dev\Http\HttpResource::safeRequest() has parameter $query with no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
92-92: Parameter #2 $query of callable BEAR\Dev\QueryMerger expects array<string, mixed>, array given.
(argument.type)
100-100: Method BEAR\Dev\Http\HttpResource::unsafeRequest() has parameter $query with no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
102-102: Parameter #2 $query of callable BEAR\Dev\QueryMerger expects array<string, mixed>, array given.
(argument.type)
137-137: Method BEAR\Dev\Http\HttpResource::execWithProcOpen() return type has no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
151-151: Cannot access offset 0 on mixed.
(offsetAccess.nonOffsetAccessible)
151-151: Parameter #1 $stream of function fclose expects resource, mixed given.
(argument.type)
154-154: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
154-154: Parameter #1 $stream of function feof expects resource, mixed given.
(argument.type)
155-155: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
155-155: Parameter #1 $stream of function fgets expects resource, mixed given.
(argument.type)
163-163: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
163-163: Parameter #1 $stream of function fclose expects resource, mixed given.
(argument.type)
164-164: Cannot access offset 2 on mixed.
(offsetAccess.nonOffsetAccessible)
164-164: Parameter #1 $stream of function fclose expects resource, mixed given.
(argument.type)
170-170: Method BEAR\Dev\Http\HttpResource::execWithExec() return type has no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
178-178: Method BEAR\Dev\Http\HttpResource::log() has parameter $output with no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
🪛 GitHub Check: codecov/patch
src/Http/HttpResource.php
[warning] 122-122: src/Http/HttpResource.php#L122
Added line #L122 was not covered by tests
[warning] 126-126: src/Http/HttpResource.php#L126
Added line #L126 was not covered by tests
[warning] 147-147: src/Http/HttpResource.php#L147
Added line #L147 was not covered by tests
[warning] 170-170: src/Http/HttpResource.php#L170
Added line #L170 was not covered by tests
[warning] 172-173: src/Http/HttpResource.php#L172-L173
Added lines #L172 - L173 were not covered by tests
[warning] 175-175: src/Http/HttpResource.php#L175
Added line #L175 was not covered by tests
[warning] 212-212: src/Http/HttpResource.php#L212
Added line #L212 was not covered by tests
[warning] 217-217: src/Http/HttpResource.php#L217
Added line #L217 was not covered by tests
[warning] 222-222: src/Http/HttpResource.php#L222
Added line #L222 was not covered by tests
[warning] 224-224: src/Http/HttpResource.php#L224
Added line #L224 was not covered by tests
[warning] 227-227: src/Http/HttpResource.php#L227
Added line #L227 was not covered by tests
[warning] 229-229: src/Http/HttpResource.php#L229
Added line #L229 was not covered by tests
[warning] 232-232: src/Http/HttpResource.php#L232
Added line #L232 was not covered by tests
[warning] 234-234: src/Http/HttpResource.php#L234
Added line #L234 was not covered by tests
[warning] 237-237: src/Http/HttpResource.php#L237
Added line #L237 was not covered by tests
[warning] 239-239: src/Http/HttpResource.php#L239
Added line #L239 was not covered by tests
🪛 GitHub Check: sa / PHPStan
src/Http/HttpResource.php
[failure] 90-90:
Method BEAR\Dev\Http\HttpResource::safeRequest() has parameter $query with no value type specified in iterable type array.
🪛 GitHub Check: cs / Coding Standards
src/Http/HttpResource.php
[failure] 90-90:
Method \BEAR\Dev\Http\HttpResource::safeRequest() does not have @param annotation for its traversable parameter $query.
[failure] 100-100:
Method \BEAR\Dev\Http\HttpResource::unsafeRequest() does not have @param annotation for its traversable parameter $query.
[failure] 137-137:
Method \BEAR\Dev\Http\HttpResource::execWithProcOpen() does not have @return annotation for its traversable return value.
[failure] 170-170:
Method \BEAR\Dev\Http\HttpResource::execWithExec() does not have @return annotation for its traversable return value.
[failure] 178-178:
Method \BEAR\Dev\Http\HttpResource::log() does not have @param annotation for its traversable parameter $output.
[failure] 186-186:
Method \BEAR\Dev\Http\HttpResource::get() does not have @param annotation for its traversable parameter $query.
[failure] 186-186:
You must use "/**" style comments for a function comment
[failure] 191-191:
Method \BEAR\Dev\Http\HttpResource::post() does not have @param annotation for its traversable parameter $query.
[failure] 196-196:
Method \BEAR\Dev\Http\HttpResource::put() does not have @param annotation for its traversable parameter $query.
🪛 GitHub Actions: Static Analysis
src/Http/HttpResource.php
[warning] 212-212: UnusedFormalParameter: Avoid unused parameters such as '$uri'.
[warning] 217-217: UnusedFormalParameter: Avoid unused parameters such as '$ro'.
[warning] 222-222: UnusedFormalParameter: Avoid unused parameters such as '$uri'.
[warning] 227-227: UnusedFormalParameter: Avoid unused parameters such as '$rel' and '$query'.
[warning] 232-232: UnusedFormalParameter: Avoid unused parameters such as '$uri' and '$query'.
[warning] 237-237: UnusedFormalParameter: Avoid unused parameters such as '$uri' and '$query'.
🔇 Additional comments (2)
src/Http/HttpResource.php (2)
14-14: 適切な型のインポートが追加されました！プロセス処理に必要な関数のインポートが適切に追加されており、コードの意図が明確です。
Also applies to: 16-20, 26-26, 28-29, 31-31
115-134: テストカバレッジの改善が必要です以下のコードパスのテストが不足しています：
execWithExecメソッド全体- エラーケースの処理
- プロセス実行の失敗ケース
テストケースの追加を提案させていただきますが、よろしいでしょうか？
Also applies to: 170-176
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 122-122: src/Http/HttpResource.php#L122
Added line #L122 was not covered by tests
[warning] 126-126: src/Http/HttpResource.php#L126
Added line #L126 was not covered by tests
| private function safeRequest(string $path, array $query): ResourceObject | ||
| { | ||
| throw new LogicException(); | ||
| $uri = ($this->queryMerger)($path, $query); | ||
| $queryParameter = $uri->query ? '?' . http_build_query($uri->query) : ''; | ||
| $url = sprintf('%s%s%s', $this->baseUri, $uri->path, $queryParameter); | ||
| $curl = sprintf('curl -s -i "%s"', $url); | ||
|
|
||
| return $this->request($curl, 'get', $url); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
型アノテーションの追加が必要です
safeRequestメソッドの実装は適切ですが、以下の型情報の追加が必要です：
- private function safeRequest(string $path, array $query): ResourceObject
+ /**
+ * @param array<string, mixed> $query
+ */
+ private function safeRequest(string $path, array $query): ResourceObject📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private function safeRequest(string $path, array $query): ResourceObject | |
| { | |
| throw new LogicException(); | |
| $uri = ($this->queryMerger)($path, $query); | |
| $queryParameter = $uri->query ? '?' . http_build_query($uri->query) : ''; | |
| $url = sprintf('%s%s%s', $this->baseUri, $uri->path, $queryParameter); | |
| $curl = sprintf('curl -s -i "%s"', $url); | |
| return $this->request($curl, 'get', $url); | |
| /** | |
| * @param array<string, mixed> $query | |
| */ | |
| private function safeRequest(string $path, array $query): ResourceObject | |
| { | |
| $uri = ($this->queryMerger)($path, $query); | |
| $queryParameter = $uri->query ? '?' . http_build_query($uri->query) : ''; | |
| $url = sprintf('%s%s%s', $this->baseUri, $uri->path, $queryParameter); | |
| $curl = sprintf('curl -s -i "%s"', $url); | |
| return $this->request($curl, 'get', $url); |
🧰 Tools
🪛 phpstan (2.0.3)
90-90: Method BEAR\Dev\Http\HttpResource::safeRequest() has parameter $query with no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
92-92: Parameter #2 $query of callable BEAR\Dev\QueryMerger expects array<string, mixed>, array given.
(argument.type)
🪛 GitHub Check: sa / PHPStan
[failure] 90-90:
Method BEAR\Dev\Http\HttpResource::safeRequest() has parameter $query with no value type specified in iterable type array.
🪛 GitHub Check: cs / Coding Standards
[failure] 90-90:
Method \BEAR\Dev\Http\HttpResource::safeRequest() does not have @param annotation for its traversable parameter $query.
| private function execWithProcOpen(string $curl): array | ||
| { | ||
| throw new LogicException(); | ||
| $descriptorspec = [ | ||
| ['pipe', 'r'], // stdin | ||
| ['pipe', 'w'], // stdout | ||
| ['pipe', 'w'], // stderr | ||
| ]; | ||
|
|
||
| $process = proc_open($curl, $descriptorspec, $pipes); | ||
| if (! is_resource($process)) { | ||
| return []; | ||
| } | ||
|
|
||
| // Close stdin as we don't need it | ||
| fclose($pipes[0]); | ||
|
|
||
| $output = []; | ||
| while (! feof($pipes[1])) { | ||
| $line = fgets($pipes[1]); | ||
| if ($line === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $output[] = trim($line); | ||
| } | ||
|
|
||
| fclose($pipes[1]); | ||
| fclose($pipes[2]); | ||
| proc_close($process); | ||
|
|
||
| return $output; |
There was a problem hiding this comment.
プロセス処理の改善が必要です
- リソースの型安全性が確保されていません
- エラーストリームの処理が不足しています
以下の改善を提案します：
private function execWithProcOpen(string $curl): array
{
+ /** @return array<string> */
$descriptorspec = [
['pipe', 'r'], // stdin
['pipe', 'w'], // stdout
['pipe', 'w'], // stderr
];
$process = proc_open($curl, $descriptorspec, $pipes);
if (! is_resource($process)) {
return [];
}
+ /** @var resource $stdin */
+ $stdin = $pipes[0];
+ /** @var resource $stdout */
+ $stdout = $pipes[1];
+ /** @var resource $stderr */
+ $stderr = $pipes[2];
- // Close stdin as we don't need it
- fclose($pipes[0]);
+ fclose($stdin);
$output = [];
- while (! feof($pipes[1])) {
- $line = fgets($pipes[1]);
+ while (! feof($stdout)) {
+ $line = fgets($stdout);
if ($line === false) {
continue;
}
$output[] = trim($line);
}
- fclose($pipes[1]);
- fclose($pipes[2]);
+ // エラー出力の処理を追加
+ $errors = [];
+ while (! feof($stderr)) {
+ $line = fgets($stderr);
+ if ($line === false) {
+ continue;
+ }
+ $errors[] = trim($line);
+ }
+
+ fclose($stdout);
+ fclose($stderr);
proc_close($process);
+ if ($errors !== []) {
+ throw new RuntimeException('Curl command failed: ' . implode(PHP_EOL, $errors));
+ }
return $output;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private function execWithProcOpen(string $curl): array | |
| { | |
| throw new LogicException(); | |
| $descriptorspec = [ | |
| ['pipe', 'r'], // stdin | |
| ['pipe', 'w'], // stdout | |
| ['pipe', 'w'], // stderr | |
| ]; | |
| $process = proc_open($curl, $descriptorspec, $pipes); | |
| if (! is_resource($process)) { | |
| return []; | |
| } | |
| // Close stdin as we don't need it | |
| fclose($pipes[0]); | |
| $output = []; | |
| while (! feof($pipes[1])) { | |
| $line = fgets($pipes[1]); | |
| if ($line === false) { | |
| continue; | |
| } | |
| $output[] = trim($line); | |
| } | |
| fclose($pipes[1]); | |
| fclose($pipes[2]); | |
| proc_close($process); | |
| return $output; | |
| private function execWithProcOpen(string $curl): array | |
| { | |
| /** @return array<string> */ | |
| $descriptorspec = [ | |
| ['pipe', 'r'], // stdin | |
| ['pipe', 'w'], // stdout | |
| ['pipe', 'w'], // stderr | |
| ]; | |
| $process = proc_open($curl, $descriptorspec, $pipes); | |
| if (! is_resource($process)) { | |
| return []; | |
| } | |
| /** @var resource $stdin */ | |
| $stdin = $pipes[0]; | |
| /** @var resource $stdout */ | |
| $stdout = $pipes[1]; | |
| /** @var resource $stderr */ | |
| $stderr = $pipes[2]; | |
| fclose($stdin); | |
| $output = []; | |
| while (! feof($stdout)) { | |
| $line = fgets($stdout); | |
| if ($line === false) { | |
| continue; | |
| } | |
| $output[] = trim($line); | |
| } | |
| // エラー出力の処理を追加 | |
| $errors = []; | |
| while (! feof($stderr)) { | |
| $line = fgets($stderr); | |
| if ($line === false) { | |
| continue; | |
| } | |
| $errors[] = trim($line); | |
| } | |
| fclose($stdout); | |
| fclose($stderr); | |
| proc_close($process); | |
| if ($errors !== []) { | |
| throw new RuntimeException('Curl command failed: ' . implode(PHP_EOL, $errors)); | |
| } | |
| return $output; | |
| } |
🧰 Tools
🪛 phpstan (2.0.3)
137-137: Method BEAR\Dev\Http\HttpResource::execWithProcOpen() return type has no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
151-151: Cannot access offset 0 on mixed.
(offsetAccess.nonOffsetAccessible)
151-151: Parameter #1 $stream of function fclose expects resource, mixed given.
(argument.type)
154-154: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
154-154: Parameter #1 $stream of function feof expects resource, mixed given.
(argument.type)
155-155: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
155-155: Parameter #1 $stream of function fgets expects resource, mixed given.
(argument.type)
163-163: Cannot access offset 1 on mixed.
(offsetAccess.nonOffsetAccessible)
163-163: Parameter #1 $stream of function fclose expects resource, mixed given.
(argument.type)
164-164: Cannot access offset 2 on mixed.
(offsetAccess.nonOffsetAccessible)
164-164: Parameter #1 $stream of function fclose expects resource, mixed given.
(argument.type)
🪛 GitHub Check: codecov/patch
[warning] 147-147: src/Http/HttpResource.php#L147
Added line #L147 was not covered by tests
🪛 GitHub Check: cs / Coding Standards
[failure] 137-137:
Method \BEAR\Dev\Http\HttpResource::execWithProcOpen() does not have @return annotation for its traversable return value.
| // Other interface methods with LogicException... | ||
| public function newInstance($uri): ResourceObject | ||
| { | ||
| throw new LogicException(); | ||
| } | ||
|
|
||
| /** | ||
| * {@inheritDoc} | ||
| * | ||
| * @codeCoverageIgnore | ||
| */ | ||
| public function options(string $uri, array $query = []): ResourceObject | ||
| public function object(ResourceObject $ro): RequestInterface | ||
| { | ||
| throw new LogicException(); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<string, mixed> $query | ||
| */ | ||
| private function safeRequest(string $path, array $query): ResourceObject | ||
| public function uri($uri): RequestInterface | ||
| { | ||
| $uri = ($this->queryMerger)($path, $query); | ||
| $queryParameter = $uri->query ? '?' . http_build_query($uri->query) : ''; | ||
| $url = sprintf('%s%s%s', $this->baseUri, $uri->path, $queryParameter); | ||
| $curl = sprintf("curl -s -i '%s'", $url); | ||
|
|
||
| return $this->request($curl, 'get', $url); | ||
| throw new LogicException(); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<mixed> $query | ||
| */ | ||
| private function unsafeRequest(string $method, string $path, array $query): ResourceObject | ||
| public function href(string $rel, array $query = []): ResourceObject | ||
| { | ||
| $uri = ($this->queryMerger)($path, $query); | ||
| $json = json_encode($uri->query, JSON_THROW_ON_ERROR); | ||
| $url = sprintf('%s%s', $this->baseUri, $uri->path); | ||
|
|
||
| $curl = sprintf("curl -s -i -H 'Content-Type:application/json' -X %s -d '%s' %s", $method, $json, $url); | ||
|
|
||
| return $this->request($curl, $method, $url); | ||
| throw new LogicException(); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<string> $output | ||
| */ | ||
| public function log(array $output, string $curl): void | ||
| public function head(string $uri, array $query = []): ResourceObject | ||
| { | ||
| $responseLog = implode(PHP_EOL, $output); | ||
| $log = sprintf("%s\n\n%s", $curl, $responseLog) . PHP_EOL . PHP_EOL; | ||
| file_put_contents($this->logFile, $log, FILE_APPEND); | ||
| throw new LogicException(); | ||
| } | ||
|
|
||
| public function request(string $curl, string $method, string $url): ResourceObject | ||
| public function options(string $uri, array $query = []): ResourceObject | ||
| { | ||
| exec($curl, $output); | ||
| $uri = new ResourceUri($url); | ||
| $uri->method = $method; | ||
| $ro = ($this->createResponse)($uri, $output); | ||
| $this->log($output, $curl); | ||
|
|
||
| return $ro; | ||
| throw new LogicException(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
未使用パラメータの改善が必要です
インターフェース実装メソッドで未使用パラメータが検出されました。以下の改善を提案します：
- パラメータの使用意図を明確にするためのドキュメント追加
- 未実装メソッドの理由の説明
+ /**
+ * @param mixed $uri
+ * @throws LogicException このメソッドは意図的に未実装です
+ */
public function newInstance($uri): ResourceObject
{
throw new LogicException();
}同様の改善を他の未実装メソッドにも適用してください。
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 212-212: src/Http/HttpResource.php#L212
Added line #L212 was not covered by tests
[warning] 217-217: src/Http/HttpResource.php#L217
Added line #L217 was not covered by tests
[warning] 222-222: src/Http/HttpResource.php#L222
Added line #L222 was not covered by tests
[warning] 224-224: src/Http/HttpResource.php#L224
Added line #L224 was not covered by tests
[warning] 227-227: src/Http/HttpResource.php#L227
Added line #L227 was not covered by tests
[warning] 229-229: src/Http/HttpResource.php#L229
Added line #L229 was not covered by tests
[warning] 232-232: src/Http/HttpResource.php#L232
Added line #L232 was not covered by tests
[warning] 234-234: src/Http/HttpResource.php#L234
Added line #L234 was not covered by tests
[warning] 237-237: src/Http/HttpResource.php#L237
Added line #L237 was not covered by tests
[warning] 239-239: src/Http/HttpResource.php#L239
Added line #L239 was not covered by tests
🪛 GitHub Actions: Static Analysis
[warning] 212-212: UnusedFormalParameter: Avoid unused parameters such as '$uri'.
[warning] 217-217: UnusedFormalParameter: Avoid unused parameters such as '$ro'.
[warning] 222-222: UnusedFormalParameter: Avoid unused parameters such as '$uri'.
[warning] 227-227: UnusedFormalParameter: Avoid unused parameters such as '$rel' and '$query'.
[warning] 232-232: UnusedFormalParameter: Avoid unused parameters such as '$uri' and '$query'.
[warning] 237-237: UnusedFormalParameter: Avoid unused parameters such as '$uri' and '$query'.
Add PHP 8.3 to old_stable and update current_stable to PHP 8.4. This ensures compatibility checks with the latest PHP versions.