[Bug] Create the snapshot temp directory when missing; drop checks PHPStan 2.1 rejects - #521
Merged
Merged
Conversation
…PStan 2.1 rejects Aligns 2.5 with the adaptations the forward merge of #513 needed on the 2026 lines, so the next forward merge does not diverge: - DocumentFileWriter::temporaryDirectory() creates Pimcore's system temp directory when it does not exist yet. tempnam() otherwise falls back to the system temp directory and raises a notice, which PHP 8.4 test runs turn into errors (19 unit tests failed in a fresh environment). - Remove three redundant checks (is_array() on an array, ?? on an explode() offset that always exists) that PHPStan 2.1 reports. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The temp-directory bug fix needs a deterministic regression test.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Aligns snapshot handling with newer branches by ensuring Pimcore’s temporary directory exists and satisfying PHPStan 2.1.
Changes:
- Creates the configured snapshot temp directory when missing.
- Removes redundant array and
explode()checks.
| File | Description |
|---|---|
DocumentFileWriter.php |
Creates or falls back from the configured temp directory. |
ReplayIndexSettings.php |
Removes a redundant array check. |
WorkerPoolBulkDispatcher.php |
Simplifies guaranteed explode() offset access. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The create-or-fall-back logic moves into DocumentFileWriter::usableDirectory(), which takes the directory as a parameter so it can be tested without the Pimcore constant: a missing nested directory is created and used, an existing one is used as is, an uncreatable one and no configured one fall back to the system temp. (Copilot, #521) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The relation filters used 123 as an id that does not match. Once enough elements were created earlier in the run, the test's own asset, document or object gets id 123 and matches, failing the "no hit" assertion (seen in CI on #521). Use an id above the freshly created element instead, as #464 did for the other filter tests. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot
AI
dismissed their stale review, a newer Copilot review was requested
September 24, 2026 10:46
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes in this pull request
Aligns 2.5 with the adaptations the forward merge of #513 needed on the 2026 lines (merge commit
d314f13on 2026.2), so 2.5 and the 2026 lines stay identical for the next forward merge.Temp directory:
DocumentFileWriter::temporaryDirectory()now creates Pimcore's system temp directory (PIMCORE_SYSTEM_TEMP_DIRECTORY) when it does not exist yet, and falls back to the system temp directory only if that fails. Without it,tempnam()silently falls back and raises a notice; in a fresh PHP 8.4 test environment on 2026.2 that failed 19 snapshot unit tests.PHPStan 2.1: removes three redundant checks that PHPStan 2.1 reports (
is_array()on a value that is always an array,??on anexplode()offset that always exists). Behaviour is unchanged; PHPStan 1.12 on 2.5 accepts both forms.Flaky test:
AssetMetadataFilterTest's relation filters used the literal id123as a non-matching id; once enough elements were created earlier in the run, the test's own element gets that id and matches (failed two CI legs on this PR). It now uses an id above the freshly created element, as [Test] Fix flaky functional tests caused by numeric fixtures colliding with element ids #464 did for the other filter tests.Additional info
Verified locally on 2.5: PHPStan clean, snapshot unit suite 68, command suite 19 and snapshot functional suite 37 OK. The same changes were verified on 2026.2 and 2026.3 during the forward merge (full suite 746 OK on 2026.3).
🤖 Generated with Claude Code