Track PHP logs (errors, warnings) in Applicaiton Tests#75
Track PHP logs (errors, warnings) in Applicaiton Tests#75korobochkin wants to merge 8 commits intodevelopfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a syslog-based logging pipeline for the test environment: a syslog container (rsyslog) listens on UDP 1514 and writes JSON logs to /var/log/remote; docker-compose routes WordPress container logs to that syslog and mounts a shared tmpfs volume; a WordPressServerLogs PHP service reads and parses those JSON log files into LogEntry objects and supports checkpointing; the PHPUnit bootstrap registers the service and AbstractHttpTestCase checkpoints before tests and fails tests if new error-level logs appear; a Makefile target resets the remote logs before running php_cs_fixer. Sequence DiagramsequenceDiagram
participant Test as Test Suite
participant WP as WordPress Container
participant Syslog as Syslog Container
participant Logs as /var/log/remote
participant Service as WordPressServerLogs
Test->>Service: checkpoint()
Note right of Test: run test operations
Test->>WP: perform requests/actions
WP->>Syslog: send syslog messages (UDP 127.0.0.1:1514)
Syslog->>Logs: append JSON lines to wordpress-all.log / wordpress-errors.log
Test->>Service: getErrorsSinceCheckpoint()
Service->>Logs: read log files
Service->>Service: parse JSON lines -> LogEntry[]
Service-->>Test: return new error LogEntry[]
Test->>Test: assert no new errors (fail if any)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/tests/application/containers/syslog/syslog.Dockerfile:
- Around line 1-11: The Dockerfile creates extra layers by using multiple RUN
instructions; consolidate the package install and directory creation into a
single RUN (referencing the RUN lines that call "apk add --no-cache rsyslog" and
"mkdir -p /var/log/remote") to reduce image layers, and optionally combine the
COPY, EXPOSE and CMD into logical grouping for clarity; you can leave out
HEALTHCHECK and non-root changes since they are intentionally omitted for test
infra.
In @.github/tests/application/docker-compose.yml:
- Around line 12-14: The healthcheck uses netstat (healthcheck.test) which is
not present in the alpine:3 base image; fix by either installing net-tools in
the syslog container Dockerfile or change the healthcheck.test to use the
busybox ps command to detect the rsyslog daemon (e.g., check for the rsyslogd
process via ps) so it doesn't require extra packages; update the healthcheck
block (healthcheck, test) accordingly and keep start_period as-is.
In @.github/tests/application/tests/Abstracts/AbstractHttpTestCase.php:
- Around line 59-77: The #[\Override] attribute on the assertPostConditions()
method is causing the same psalm/attribute error as setUp(); fix it the same way
by either removing the #[\Override] attribute from the method declaration or
adding a docblock line "@psalm-suppress InvalidAttribute" immediately above the
assertPostConditions() method (the method named assertPostConditions in the
AbstractHttpTestCase class) so static analysis no longer flags the attribute.
- Around line 49-54: The #[\Override] attribute on the setUp method is causing
Psalm's InvalidAttribute error; remove the #[\Override] attribute from the
protected function setUp(): void in AbstractHttpTestCase (or, if you must keep
it, add a Psalm suppression for InvalidAttribute on that method) so the CI no
longer fails—locate the setUp method in AbstractHttpTestCase and delete the
#[\Override] token (or add an inline Psalm suppression comment before the
method).
In @.github/tests/application/tests/Entities/Logs/LogEntry.php:
- Around line 23-28: The `@var` annotation for the Syslog severity ($severityCode)
is wrong because it disallows 0; update the property docblock from `@var`
positive-int to a correct range such as `@var` non-negative-int or `@var` int<0,7>,
and also update the constructor/phpdoc parameter annotation for the
corresponding constructor parameter (the __construct signature that documents
severity) to the same non-negative/int<0,7> annotation so docs and static
analysis accept 0 as a valid severity.
In @.github/tests/application/tests/Services/WordPressServerLogs.php:
- Around line 96-100: The current check casts severity to int and rejects 0
(emergency); instead ensure the input field exists and accept 0–7 as valid; so
in the block that uses $data and $severityCode, first verify the presence of
'severity_code' (e.g. with array_key_exists or isset) and throw the same
\UnexpectedValueException only when the field is missing, then cast to int into
$severityCode and validate that $severityCode is within the valid syslog range
(0 through 7) rather than checking < 1; update the validation around
$severityCode accordingly to accept 0 as valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15e18d93-896b-4ad5-b6ad-43dae94359d7
📒 Files selected for processing (8)
.github/tests/application/Makefile.github/tests/application/containers/syslog/rsyslog.conf.github/tests/application/containers/syslog/syslog.Dockerfile.github/tests/application/docker-compose.yml.github/tests/application/phpunit-bootstrap.php.github/tests/application/tests/Abstracts/AbstractHttpTestCase.php.github/tests/application/tests/Entities/Logs/LogEntry.php.github/tests/application/tests/Services/WordPressServerLogs.php
| #[\Override] | ||
| protected function setUp(): void | ||
| { | ||
| parent::setUp(); | ||
| $this->logs->checkpoint(); | ||
| } |
There was a problem hiding this comment.
Pipeline failure: Remove or suppress #[\Override] attribute for Psalm compatibility.
The CI pipeline is failing because Psalm reports InvalidAttribute: Attribute Override cannot be used on a function. This is blocking the build.
The #[\Override] attribute is a PHP 8.3+ feature that Psalm may not fully support in its current configuration. You have several options:
🔧 Option 1: Remove the attribute (recommended)
- #[\Override]
protected function setUp(): void
{
parent::setUp();
$this->logs->checkpoint();
}🔧 Option 2: Suppress Psalm error inline
/**
* `@throws` \UnexpectedValueException
+ *
+ * `@psalm-suppress` InvalidAttribute
*/
#[\Override]
protected function setUp(): void📝 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.
| #[\Override] | |
| protected function setUp(): void | |
| { | |
| parent::setUp(); | |
| $this->logs->checkpoint(); | |
| } | |
| protected function setUp(): void | |
| { | |
| parent::setUp(); | |
| $this->logs->checkpoint(); | |
| } |
🧰 Tools
🪛 GitHub Actions: Code Testing
[error] 49-49: Psalm error: InvalidAttribute: Attribute Override cannot be used on a function (see https://psalm.dev/242)
🪛 GitHub Check: Psalm. Tests Application
[failure] 49-49: InvalidAttribute
.github/tests/application/tests/Abstracts/AbstractHttpTestCase.php:49:7: InvalidAttribute: Attribute Override cannot be used on a function (see https://psalm.dev/242)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/tests/application/tests/Abstracts/AbstractHttpTestCase.php around
lines 49 - 54, The #[\Override] attribute on the setUp method is causing Psalm's
InvalidAttribute error; remove the #[\Override] attribute from the protected
function setUp(): void in AbstractHttpTestCase (or, if you must keep it, add a
Psalm suppression for InvalidAttribute on that method) so the CI no longer
fails—locate the setUp method in AbstractHttpTestCase and delete the
#[\Override] token (or add an inline Psalm suppression comment before the
method).
There was a problem hiding this comment.
Previously Psalm did not complain about #[\Override] in that code. It feels like a bug in mix of current PHP + Psalm versions.
There was a problem hiding this comment.
🧩 Analysis chain
Good question — this is a well-known compatibility issue. Let me check your Psalm configuration and composer.json to find the root cause.
🏁 Script executed:
#!/bin/bash
# Find psalm config files
fd -e xml psalm
# Find composer.json in the tests/application directory
fd composer.json .github/tests/application --max-depth 2
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check the composer.json for PHP version and psalm config
cat .github/tests/application/composer.json
# Check Makefile for psalm invocation
grep -n -A3 -B3 'psalm' .github/tests/application/Makefile
Length of output: 688
🏁 Script executed:
#!/bin/bash
# Look for Phive config
fd phars.xml
fd -e xml . .phive 2>/dev/null
cat .phive/phars.xml 2>/dev/null
# Look for psalm in GitHub Actions workflows
rg -n 'psalm' .github/workflows/ -A3 -B3
# Look for any psalm config file (not just .xml extension)
fd psalm
Length of output: 4672
🏁 Script executed:
#!/bin/bash
# Check the psalm.xml config
cat .github/psalm/psalm.xml 2>/dev/null || fd psalm.xml .github/psalm --exec cat {}
# Check the psalm composer.json
cat .github/psalm/composer.json 2>/dev/null
# Check Makefile for psalm-tests-application target
grep -n -A10 'psalm-tests-application' .github/tests/application/Makefile 2>/dev/null
grep -n -A10 'psalm-tests-application' Makefile 2>/dev/null || true
Length of output: 2668
🏁 Script executed:
#!/bin/bash
cat .github/psalm/psalm-tests-application.xml
Length of output: 1089
@korobochkin, this is not a bug in PHP or Psalm themselves — it's a missing phpVersion declaration in .github/psalm/psalm-tests-application.xml. Here's why:
psalm-tests-application.xmlhas nophpVersionattribute.- The tests
composer.jsonhas no PHP version in itsrequireblock either. - When Psalm can't determine the target PHP version from either source, it falls back to a default well below PHP 8.3.
- Since
#[\Override]is a PHP 8.3+ built-in attribute, Psalm raisesInvalidAttributewhen it targets an older version.
The fix is to explicitly set phpVersion="8.4" in psalm-tests-application.xml (8.4 rather than 8.3 because your code already uses property hooks — e.g. protected BrowsersService $browsers { get { ... } } — which are PHP 8.4):
<psalm
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
errorLevel="1"
resolveFromConfigFile="false"
xmlns="https://getpsalm.org/schema/config"
xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
+ phpVersion="8.4"
findUnusedBaselineEntry="true"Optionally, you can also lock the PHP version in the tests composer.json to keep both in sync:
+ "require": {
+ "php": "^8.4"
+ },
"require-dev": {This is also the reason it "worked before" — the #[\Override] attributes on other methods were introduced in this PR for the first time in the tests suite, so Psalm had never been confronted with it in the psalm-tests-application job until now.
There was a problem hiding this comment.
It's strange. I tried to add the PHP version as you suggested but the error is still there. There are also other related errors in Psalm such as "Error: .github/tests/application/tests/Abstracts/AbstractHttpTestCase.php:49:7: InvalidAttribute: Attribute Override cannot be used on a function (see https://psalm.dev/242)". It feels like some parsing issues for me. Maybe Psalm cannot correctly determine parts of code...
There was a problem hiding this comment.
@korobochkin, you're correct — this is essentially a parsing issue in Psalm. Here's the full picture:
The root cause is that Psalm has incomplete support for PHP 8.4 property hooks. The get { ... } accessor blocks inside the hooked properties ($browsers, $logs) are being mis-parsed by Psalm's AST engine. Psalm 6.0 has an open issue where simple property hooks cause an Unrecognized functionlike exception, meaning Psalm may interpret the get { } body as a standalone function rather than a property accessor. As a result, the class structure gets garbled after the property hook definitions — and #[\Override] on setUp() (which comes after both hooked properties) ends up looking to Psalm like it's decorating a bare function, triggering InvalidAttribute: Attribute Override cannot be used on a function.
Setting phpVersion="8.4" correctly teaches Psalm that #[\Override] is a valid built-in attribute (fixing the older "class Override doesn't have the Attribute attribute" error), but it doesn't fix the property hooks parsing problem that's disrupting the class body layout.
The Psalm maintainer has noted that property hook support is "coming within the next few releases", so this is a known gap.
Pragmatic workaround — suppress the false positive inline:
/**
* `@throws` \UnexpectedValueException
* `@psalm-suppress` InvalidAttribute
*/
#[\Override]
protected function setUp(): void
{
parent::setUp();
$this->logs->checkpoint();
}
/**
* `@throws` \UnexpectedValueException
* `@psalm-suppress` InvalidAttribute
*/
#[\Override]
protected function assertPostConditions(): void
{
// ...
}This keeps the #[\Override] attributes (which are genuinely correct PHP 8.3+/8.4 code) and suppresses the false positive until Psalm's property hooks support matures enough to parse the class correctly.
Alternatively, if you'd prefer to not suppress, you could also add an InvalidAttribute entry to the Psalm baseline file rather than per-method suppressions.
( ˶°ㅁ°) !!
✏️ Learnings added
Learnt from: korobochkin
Repo: korobochkin/bbpress-permalinks-with-id PR: 75
File: .github/tests/application/tests/Abstracts/AbstractHttpTestCase.php:49-54
Timestamp: 2026-03-22T14:57:43.181Z
Learning: In the bbpress-permalinks-with-id repo, Psalm has incomplete PHP 8.4 property hooks support. When a class uses PHP 8.4 property hook syntax (`protected Type $prop { get { ... } }`), Psalm mis-parses the class body, causing `#[\Override]` on methods defined after the hooked properties to trigger `InvalidAttribute: Attribute Override cannot be used on a function`. The fix is to add `psalm-suppress InvalidAttribute` to those methods rather than removing `#[\Override]`. Setting `phpVersion="8.4"` in `psalm-tests-application.xml` is necessary but not sufficient on its own.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: korobochkin
Repo: korobochkin/bbpress-permalinks-with-id PR: 6
File: plugin.php:66-75
Timestamp: 2025-09-06T15:23:45.720Z
Learning: In version 1.0.6 of bbpress-permalinks-with-id, the maintainer korobochkin prefers to keep minimal changes to the PHP codebase, focusing primarily on repository structure updates and CI/analysis tool additions. Type safety improvements are planned for future versions.
korobochkin
force-pushed
the
feature/php-logs
branch
from
3f7355c to
f5d1402
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/psalm/psalm-tests-application.xml:
- Line 8: The psalm-tests-application.xml declares phpVersion="8.4" while CI
runs PHP 8.5 and psalm.xml has no version; update phpVersion in
psalm-tests-application.xml to "8.5" to match the workflow (or alternatively
change the CI runner to 8.4) and/or add a clear comment in
psalm-tests-application.xml documenting the intentional divergence; ensure any
change is mirrored or documented in psalm.xml so static analysis runs use a
consistent PHP target across tests and production.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: af3ace8e-a30e-43b7-83e5-6f6ef45bb1f5
⛔ Files ignored due to path filters (1)
.github/psalm/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
.github/psalm/composer.json.github/psalm/psalm-tests-application.xml
| resolveFromConfigFile="false" | ||
| xmlns="https://getpsalm.org/schema/config" | ||
| xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd" | ||
| phpVersion="8.4" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Psalm configs and phpVersion =="
fd 'psalm.*\.xml$' .github/psalm -x sh -c 'echo "--- $1"; rg -n "phpVersion|<psalm" "$1"' sh {}
echo
echo "== Workflow PHP versions around Psalm jobs =="
rg -n -C2 'psalm|setup-php|php-version' .github/workflowsRepository: korobochkin/bbpress-permalinks-with-id
Length of output: 7077
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full content of both Psalm config files (first 15 lines) ==="
echo "--- psalm.xml ---"
head -15 .github/psalm/psalm.xml
echo
echo "--- psalm-tests-application.xml ---"
head -15 .github/psalm/psalm-tests-application.xml
echo
echo "=== Check for any documentation about this setup ==="
find .github -name "*.md" -o -name "*.txt" | xargs grep -l "psalm\|phpVersion" 2>/dev/null || echo "No doc files found with psalm references"Repository: korobochkin/bbpress-permalinks-with-id
Length of output: 1395
Align or document phpVersion for tests analysis.
The psalm-tests-application.xml config declares phpVersion="8.4" (line 8), but the corresponding workflow job runs on PHP 8.5. Meanwhile, the production config (psalm.xml) has no version constraint. Either align the declared versions with the actual runtime environments or document why this divergence is intentional, to prevent unexpected static-analysis inconsistencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/psalm/psalm-tests-application.xml at line 8, The
psalm-tests-application.xml declares phpVersion="8.4" while CI runs PHP 8.5 and
psalm.xml has no version; update phpVersion in psalm-tests-application.xml to
"8.5" to match the workflow (or alternatively change the CI runner to 8.4)
and/or add a clear comment in psalm-tests-application.xml documenting the
intentional divergence; ensure any change is mirrored or documented in psalm.xml
so static analysis runs use a consistent PHP target across tests and production.
cc0cb3e to
99edd08
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/tests/application/tests/Services/WordPressServerLogs.php (1)
96-100:⚠️ Potential issue | 🟡 MinorSeverity code validation improved but missing field detection is inconsistent.
The range validation now correctly accepts 0–7, but unlike other required fields (
timestamp,severity,tag), a missingseverity_codesilently defaults to 0 (emergency) rather than throwing. This inconsistency could mask malformed log records.🔧 Proposed fix for consistent field validation
- $severityCode = (int) ($data['severity_code'] ?? 0); + if (!array_key_exists('severity_code', $data)) { + throw new \UnexpectedValueException('Missing "severity_code" in log record'); + } + + $severityCode = (int) $data['severity_code']; if ($severityCode < 0 || $severityCode > 7) { throw new \UnexpectedValueException('Bad "severity_code" in log record'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/tests/application/tests/Services/WordPressServerLogs.php around lines 96 - 100, The code silently defaults missing severity_code to 0; update the validation in the WordPressServerLogs parser so missing severity_code is treated as an error like other required fields: check for the existence of $data['severity_code'] (e.g., using array_key_exists or isset) before casting to (int) into $severityCode, and if absent throw a \UnexpectedValueException('Missing "severity_code" in log record'); keep the existing range check on $severityCode (0–7) as-is to validate value bounds..github/tests/application/tests/Abstracts/AbstractHttpTestCase.php (1)
56-77:⚠️ Potential issue | 🟠 MajorAdd
@psalm-suppress InvalidAttributehere as well.Same Psalm/PHP 8.4 property hooks issue as
setUp().🔧 Proposed fix
/** * `@throws` \UnexpectedValueException + * + * `@psalm-suppress` InvalidAttribute */ #[\Override] protected function assertPostConditions(): void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/tests/application/tests/Abstracts/AbstractHttpTestCase.php around lines 56 - 77, The assertPostConditions method in AbstractHttpTestCase needs the same Psalm/PHP 8.4 property hooks suppression applied as setUp; add the `@psalm-suppress` InvalidAttribute annotation to the method's docblock (above protected function assertPostConditions) so Psalm won't flag the attribute-related false positive—mirror the change made to setUp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/tests/application/tests/Abstracts/AbstractHttpTestCase.php:
- Around line 46-54: Add the Psalm suppression to the setUp method to silence
the false positive InvalidAttribute error: in the AbstractHttpTestCase class, on
the method setUp() that currently has the attribute #[\Override], add the
docblock annotation `@psalm-suppress` InvalidAttribute above the method
(preserving the existing #[\Override] attribute and the throws annotation) so
Psalm no longer fails CI for this known PHP 8.4 property hook parsing bug.
---
Duplicate comments:
In @.github/tests/application/tests/Abstracts/AbstractHttpTestCase.php:
- Around line 56-77: The assertPostConditions method in AbstractHttpTestCase
needs the same Psalm/PHP 8.4 property hooks suppression applied as setUp; add
the `@psalm-suppress` InvalidAttribute annotation to the method's docblock (above
protected function assertPostConditions) so Psalm won't flag the
attribute-related false positive—mirror the change made to setUp.
In @.github/tests/application/tests/Services/WordPressServerLogs.php:
- Around line 96-100: The code silently defaults missing severity_code to 0;
update the validation in the WordPressServerLogs parser so missing severity_code
is treated as an error like other required fields: check for the existence of
$data['severity_code'] (e.g., using array_key_exists or isset) before casting to
(int) into $severityCode, and if absent throw a
\UnexpectedValueException('Missing "severity_code" in log record'); keep the
existing range check on $severityCode (0–7) as-is to validate value bounds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70b084f7-b6af-48a6-b5dd-77c76e76d4e5
📒 Files selected for processing (3)
.github/tests/application/tests/Abstracts/AbstractHttpTestCase.php.github/tests/application/tests/Entities/Logs/LogEntry.php.github/tests/application/tests/Services/WordPressServerLogs.php
| /** | ||
| * @throws \UnexpectedValueException | ||
| */ | ||
| #[\Override] | ||
| protected function setUp(): void | ||
| { | ||
| parent::setUp(); | ||
| $this->logs->checkpoint(); | ||
| } |
There was a problem hiding this comment.
Add @psalm-suppress InvalidAttribute to resolve CI failure.
Per the learnings, Psalm mis-parses PHP 8.4 property hooks, causing #[\Override] on methods declared after hooked properties to trigger InvalidAttribute. The workaround is to keep #[\Override] and add the suppression.
🔧 Proposed fix
/**
* `@throws` \UnexpectedValueException
+ *
+ * `@psalm-suppress` InvalidAttribute
*/
#[\Override]
protected function setUp(): voidBased on learnings: "In this repository, Psalm currently mis-parses PHP 8.4 property hooks... As a workaround, keep #[\Override] and add psalm-suppress InvalidAttribute to any methods declared after hooked properties."
🧰 Tools
🪛 GitHub Check: Psalm. Tests Application
[failure] 49-49: InvalidAttribute
.github/tests/application/tests/Abstracts/AbstractHttpTestCase.php:49:7: InvalidAttribute: Attribute Override cannot be used on a function (see https://psalm.dev/242)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/tests/application/tests/Abstracts/AbstractHttpTestCase.php around
lines 46 - 54, Add the Psalm suppression to the setUp method to silence the
false positive InvalidAttribute error: in the AbstractHttpTestCase class, on the
method setUp() that currently has the attribute #[\Override], add the docblock
annotation `@psalm-suppress` InvalidAttribute above the method (preserving the
existing #[\Override] attribute and the throws annotation) so Psalm no longer
fails CI for this known PHP 8.4 property hook parsing bug.
korobochkin
force-pushed
the
feature/php-logs
branch
from
99edd08 to
e69e32e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/tests/application/tests/Abstracts/AbstractHttpTestCase.php (1)
46-60:⚠️ Potential issue | 🔴 CriticalAdd Psalm's inline workaround to both overridden hooks.
These two
#[\Override]methods still match the known false-positive pattern in this repo: they sit after property hooks, and Psalm is already failing on both lines. Keep#[\Override], but add@psalm-suppress InvalidAttributeabove each method.🔧 Proposed fix
/** * `@throws` \UnexpectedValueException + * + * `@psalm-suppress` InvalidAttribute */ #[\Override] protected function setUp(): void @@ /** * `@throws` \UnexpectedValueException + * + * `@psalm-suppress` InvalidAttribute */ #[\Override] protected function assertPostConditions(): voidRun this to confirm both methods still lack the inline suppression:
#!/bin/bash rg -n -C2 '@psalm-suppress InvalidAttribute|#\[\\Override\]|function (setUp|assertPostConditions)\b|protected .* \$[a-z]+ \{' \ .github/tests/application/tests/Abstracts/AbstractHttpTestCase.phpBased on learnings: In this repository, Psalm currently mis-parses PHP 8.4 property hooks; keep
#[\Override]and add@psalm-suppress InvalidAttributeto methods declared after hooked properties that trigger this false positive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/tests/application/tests/Abstracts/AbstractHttpTestCase.php around lines 46 - 60, Add the inline Psalm suppression above both overridden hooks: keep the existing attribute #[\Override] on the methods setUp() and assertPostConditions(), and add the docblock annotation `@psalm-suppress` InvalidAttribute immediately above each method declaration so Psalm stops flagging the known false-positive for methods declared after property hooks..github/tests/application/tests/Services/WordPressServerLogs.php (1)
96-100:⚠️ Potential issue | 🟡 MinorValidate the raw
severity_codebefore casting.
($data['severity_code'] ?? 0)plus(int)turns a missing key,null, or any non-numeric value into0, so malformed records are accepted asemergency. Validate the raw value and range first.🔧 Proposed fix
- $severityCode = (int) ($data['severity_code'] ?? 0); - - if ($severityCode < 0 || $severityCode > 7) { + $severityCode = filter_var( + $data['severity_code'] ?? null, + FILTER_VALIDATE_INT, + ['options' => ['min_range' => 0, 'max_range' => 7]], + ); + + if (false === $severityCode) { throw new \UnexpectedValueException('Bad "severity_code" in log record'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/tests/application/tests/Services/WordPressServerLogs.php around lines 96 - 100, Validate the raw $data['severity_code'] before casting instead of casting first: check that the key exists, is not null, and is a numeric value (e.g., is_numeric) and then parse it to an int into $severityCode; if the raw value is missing or non-numeric or the resulting int is outside 0–7, throw the existing \UnexpectedValueException('Bad "severity_code" in log record'). Update the logic around $severityCode and $data['severity_code'] to perform these checks prior to casting so malformed records are rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/tests/application/Makefile:
- Around line 13-14: The reset_logs Makefile target currently runs docker exec
syslog sh -c "truncate -s 0 /var/log/remote/*.log" which can pass a literal
*.log to truncate when no files exist; change the command in the reset_logs
target (inside the same Makefile, refer to the reset_logs target and the docker
exec syslog invocation) to guard the glob—e.g., use find /var/log/remote -name
'*.log' -exec truncate -s 0 {} + or a shell loop that checks for matches before
truncating—so the target is idempotent and doesn’t create or operate on a
literal '*.log' filename.
In @.github/tests/application/tests/Services/WordPressServerLogs.php:
- Around line 70-74: When file($path, FILE_IGNORE_NEW_LINES |
FILE_SKIP_EMPTY_LINES) returns false, don't silently return []; instead surface
the failure by throwing an exception (e.g., RuntimeException) that includes the
$path and any available error details (use error_get_last() if needed) so
callers/tests become aware the harness lost visibility into WordPress logs;
update the block that checks "if (false === $lines)" to throw this informative
exception rather than returning an empty array.
---
Duplicate comments:
In @.github/tests/application/tests/Abstracts/AbstractHttpTestCase.php:
- Around line 46-60: Add the inline Psalm suppression above both overridden
hooks: keep the existing attribute #[\Override] on the methods setUp() and
assertPostConditions(), and add the docblock annotation `@psalm-suppress`
InvalidAttribute immediately above each method declaration so Psalm stops
flagging the known false-positive for methods declared after property hooks.
In @.github/tests/application/tests/Services/WordPressServerLogs.php:
- Around line 96-100: Validate the raw $data['severity_code'] before casting
instead of casting first: check that the key exists, is not null, and is a
numeric value (e.g., is_numeric) and then parse it to an int into $severityCode;
if the raw value is missing or non-numeric or the resulting int is outside 0–7,
throw the existing \UnexpectedValueException('Bad "severity_code" in log
record'). Update the logic around $severityCode and $data['severity_code'] to
perform these checks prior to casting so malformed records are rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0a63d93-bb29-4344-81d5-8a37fd4338ce
📒 Files selected for processing (8)
.github/tests/application/Makefile.github/tests/application/containers/syslog/rsyslog.conf.github/tests/application/containers/syslog/syslog.Dockerfile.github/tests/application/docker-compose.yml.github/tests/application/phpunit-bootstrap.php.github/tests/application/tests/Abstracts/AbstractHttpTestCase.php.github/tests/application/tests/Entities/Logs/LogEntry.php.github/tests/application/tests/Services/WordPressServerLogs.php
| reset_logs: | ||
| docker exec syslog sh -c "truncate -s 0 /var/log/remote/*.log" |
There was a problem hiding this comment.
Guard the glob so reset_logs stays idempotent.
If /var/log/remote has no .log files yet, /bin/sh passes the pattern literally to truncate, so this target either fails or creates a stray *.log file. Use find or a guarded loop instead.
🔧 Proposed fix
reset_logs:
- docker exec syslog sh -c "truncate -s 0 /var/log/remote/*.log"
+ docker exec syslog sh -c "find /var/log/remote -maxdepth 1 -type f -name '*.log' -exec truncate -s 0 {} \;"📝 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.
| reset_logs: | |
| docker exec syslog sh -c "truncate -s 0 /var/log/remote/*.log" | |
| reset_logs: | |
| docker exec syslog sh -c "find /var/log/remote -maxdepth 1 -type f -name '*.log' -exec truncate -s 0 {} \;" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/tests/application/Makefile around lines 13 - 14, The reset_logs
Makefile target currently runs docker exec syslog sh -c "truncate -s 0
/var/log/remote/*.log" which can pass a literal *.log to truncate when no files
exist; change the command in the reset_logs target (inside the same Makefile,
refer to the reset_logs target and the docker exec syslog invocation) to guard
the glob—e.g., use find /var/log/remote -name '*.log' -exec truncate -s 0 {} +
or a shell loop that checks for matches before truncating—so the target is
idempotent and doesn’t create or operate on a literal '*.log' filename.
| $lines = file($path, FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES); | ||
|
|
||
| if (false === $lines) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Don't swallow log-read failures.
Once the file exists, file() returning false means the harness lost visibility into WordPress logs. Returning [] makes the suite pass while blind to PHP warnings/errors.
🔧 Proposed fix
if (false === $lines) {
- return [];
+ throw new \UnexpectedValueException(sprintf('Failed to read log file "%s".', $path));
}📝 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.
| $lines = file($path, FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES); | |
| if (false === $lines) { | |
| return []; | |
| } | |
| $lines = file($path, FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES); | |
| if (false === $lines) { | |
| throw new \UnexpectedValueException(sprintf('Failed to read log file "%s".', $path)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/tests/application/tests/Services/WordPressServerLogs.php around
lines 70 - 74, When file($path, FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES)
returns false, don't silently return []; instead surface the failure by throwing
an exception (e.g., RuntimeException) that includes the $path and any available
error details (use error_get_last() if needed) so callers/tests become aware the
harness lost visibility into WordPress logs; update the block that checks "if
(false === $lines)" to throw this informative exception rather than returning an
empty array.
1 participant
When CI runs the Application Tests, the test suite in the
phpcontainer checks and validates status codes and HTML content in HTTP responses. During the execution and rendering of HTML pages, thewordpresscontainer and its own PHP workers might produce warnings and errors. Before this PR, there was no tracking mechanism for those warnings and errors in tests.This PR adds an extra container
syslogthat acts as a log server for thewordpresscontainer instead of its defaultstderrstream. Then the container that runs the tests (php) checks if there are any errors after each test withAbstractHttpTestCase.assertPostConditions.