Regenerated PHPStan baseline #658
Conversation
Steveb-p
force-pushed
the
fixed-phpstan
branch
from
a06abad to
6ef1e67
Compare
|
There was a problem hiding this comment.
Pull Request Overview
Purpose: Regenerate and reorganize PHPStan baselines while introducing runtime checks and stricter typing/defensive code around DOM and cURL operations to eliminate previous baseline suppressions.
- Added runtime validation and safer DOM node handling in DoctrineDatabase gateway.
- Hardened cURL setup (error checks, non-empty URL assertions) in HTTPHandler and RemoteProvider; introduced new PHPStan baseline version segmentation.
- Replaced multiple version‐specific baseline files with consolidated gte/lte variant logic and added a new gte-8.1 baseline.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabase.php | Added DOM query error handling and safer node removal logic to satisfy static analysis. |
| src/bundle/Core/URLChecker/Handler/HTTPHandler.php | Added defensive checks around cURL initialization and URL validity; adjusted SSL option typing. |
| src/bundle/Core/Imagine/PlaceholderProvider/RemoteProvider.php | Added validations and refactored cURL/file handling for static analysis compliance. |
| phpstan-baseline.neon.php | Reworked inclusion logic for versioned baselines (added gte-8.1 include, removed per-minor explicit includes). |
| phpstan-baseline.neon | Removed obsolete ignoreErrors entries now covered or no longer needed. |
| phpstan-baseline-gte-8.1.neon | New baseline file for PHP ≥ 8.1. |
| phpstan-baseline-gte-8.0.neon | Adjusted ignoreErrors to reflect updated return type guarantees. |
| phpstan-baseline-8.2.neon | Removed legacy baseline file (superseded by new scheme). |
| phpstan-baseline-8.1.neon | Removed legacy baseline file (superseded by new scheme). |
| phpstan-baseline-8.0.neon | Removed legacy baseline file (superseded by new scheme). |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| $handler = \curl_init(); | ||
| if ($handler === false) { | ||
| throw new RuntimeException('Unable to initialize cURL.'); | ||
| } | ||
|
|
||
| $timeout = $options['timeout']; | ||
|
|
||
| curl_setopt_array($handler, [ | ||
| CURLOPT_URL => $placeholderUrl, // non-empty-string | ||
| CURLOPT_FILE => $fp, // resource | ||
| CURLOPT_TIMEOUT => $timeout, // int | ||
| CURLOPT_FAILONERROR => true, // bool | ||
| ]); | ||
|
|
||
| try { | ||
| if (curl_exec($handler) === false) { | ||
| throw new RuntimeException("Unable to download placeholder for {$value->id} ($placeholderUrl): " . curl_error($handler)); | ||
| throw new RuntimeException( | ||
| "Unable to download placeholder for {$value->id} ({$placeholderUrl}): " . curl_error($handler) | ||
| ); | ||
| } | ||
| } finally { | ||
| curl_close($handler); |
There was a problem hiding this comment.
Resource leak: if curl_init() fails (line 42 -> 43 throws) the previously opened file handle $fp (line 36) is never closed. Wrap from the fopen() onward in a try/finally (or close $fp immediately before throwing) to ensure fclose($fp) on every exceptional path.
| throw new RuntimeException( | ||
| "Unable to download placeholder for {$value->id} ({$placeholderUrl}): " . curl_error($handler) | ||
| ); | ||
| } |
There was a problem hiding this comment.
[nitpick] On download failure (lines 56–59) a partially written temp file is left on disk. Consider unlink($path) in the error path (before rethrow) to avoid leaving corrupt placeholders.
| } | |
| } | |
| } catch (\Throwable $e) { | |
| // Remove the partially written temp file on error | |
| if (file_exists($path)) { | |
| @unlink($path); | |
| } | |
| throw $e; |
|
|
||
| foreach ($relationItems as $relationItem) { | ||
| $relationItem->parentNode->removeChild($relationItem); | ||
| if ($relationItem instanceof DOMElement && $relationItem->parentNode !== null) { |
There was a problem hiding this comment.
[nitpick] The XPath expression selects elements, so each node should already be a DOMElement; the instanceof check adds redundant branching. Removing the instanceof (keeping only a parentNode null check) would simplify the loop without changing behavior.
| if ($relationItem instanceof DOMElement && $relationItem->parentNode !== null) { | |
| if ($relationItem->parentNode !== null) { |
| if (PHP_VERSION_ID >= 80100) { | ||
| $includes[] = __DIR__ . '/phpstan-baseline-gte-8.1.neon'; | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] Given later conditions also include both gte-* and lte-* files for overlapping version ranges, PHP 8.1.x will load both gte-8.1 and lte-8.1 (and 8.2+ will layer multiple gte baselines). Consider clarifying or deduplicating inclusion order to avoid redundant ignoreErrors merging.
|
You can merge up changes @Steveb-p |
|
Merged up into |
6 participants
Description:
For QA:
Documentation: