DE-178815 Modernize CI + upgrade to PHP 8.2/8.4, minimal style churn

[jzajac-nice]

Description: Modernize the CI workflow and upgrade to PHP 8.2/8.4, fixing the code/tests so the suite is green — with no bulk restyle
Possible impact: CI/CD (GitHub Actions), local dev docker, Client apiVersion handling, Bulk/ResultSet builders, test suite

---

Summary

Pipelines on opensearch were stuck queueing forever on the retired ubuntu-20.04 runner. Unsticking them exposed a cascade (retired actions, dropped docker-compose, the in-flight OpenSearch apiVersion refactor left callers/tests out of sync). This takes the suite from never-running / all-red to green.

Built with Claude Code. Replaces #41 (rebuilt with clean history and minimal formatting).

ⓘ For reviewers — why the diff spans ~40 PHP files

The changes fall into two independent buckets:

1. apiVersion refactor sync (~16 files) — the actual fix: production code + tests brought in line with the OpenSearch apiVersion refactor (see below).
2. Pre-existing lint debt (~24 files) — the opensearch branch's own refactor code was never linted: CI had been stuck queueing, so php-cs-fixer never ran. Running the branch's own pinned php-cs-fixer 3.8.0 now flags 24 files that were already non-compliant on opensearch before this PR. These are mechanical style-only fixes (phpdoc, imports, whitespace), unrelated to the PHP upgrade, and are required for the coding-style gate to pass (the gate lints all of src/ + tests/).

There is no new/bulk restyle: the style gate stays on the original php-cs-fixer 3.8.0 (run under PHP 8.1, lint-only), so the ruleset is unchanged — the only style changes are the branch's own latent 3.8.0 violations.

CI / infrastructure

• Runner: ubuntu-20.04 (removed by GitHub) → ubuntu-latest; PHPUnit job pins ubuntu-22.04 (the ES 7.15.2 docker cluster crashes on 24.04).
• Actions: actions/cache@v2 (hard-failed) + checkout@v2 + codecov@v2 → v4.
• ES setup: docker-compose (gone from the new image) → docker compose v2 with --project-name=docker so the docker_elastic network still matches the health-check.
• composer-normalize: drop the ghs_ token setup-php injects (needs no auth).
• Style gate: kept on php-cs-fixer 3.8.0 under PHP 8.1 (lint only); test matrix is 8.2/8.4.

PHP 8.2 / 8.4

• PHPUnit matrix 7.2–8.1 → 8.2 + 8.4; PHPStan gate on 8.2.
• Local dev container based on the company PHP 8.2 image (docker/php/Dockerfile).

Code / test fixes (apiVersion refactor sync)

• Client: accept array|string $config; real bool from shouldLog*(); getApiVersion() defaults to API_VERSION_7 (was null → TypeError outside ClientFactory).
• Bulk\Response: accept the apiVersion the caller already passes.
• Threaded apiVersion / resolver / logger through the affected tests.
• Removed PHPUnit deprecation failures + fixed pre-existing functional failures (geo delta, logger assertions).
• Normalized composer.json; dropped an unmatched phpstan-baseline ignore.

Test Plan

• Coding style — green (local, php-cs-fixer 3.8.0)
• PHPStan — green
• PHPUnit unit — green (565 tests)
• Full CI incl. functional — verifying on this PR

[Copilot]

🟡 Changes recommended

Unresolved CI, local development, API compatibility, and Guzzle compatibility findings remain.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Modernizes CI and local tooling for PHP 8.2/8.4 while synchronizing API-version handling across production code and tests.

Changes:

• Updates GitHub Actions, runners, Docker Compose, and PHP tooling.
• Threads API-version data through clients, bulk actions, and result sets.
• Applies compatibility, lint, PHPUnit, and PHPStan fixes.

File summaries

File	Summary
tests/Transport/HttpTest.php	Updates result-set construction with API version.
tests/Transport/GuzzleTest.php	Updates Guzzle setup and API-version usage.
tests/ResultSet/ProcessingBuilderTest.php	Updates API-version expectations.
tests/ResultSet/ChainProcessorTest.php	Supplies API version to result sets.
tests/ResultSet/BuilderTest.php	Updates builder calls.
tests/Multi/MultiBuilderTest.php	Updates multi-result builder expectations.
tests/Exception/PartialShardFailureExceptionTest.php	Updates partial-failure result construction.
tests/DocumentTest.php	Replaces deprecated PHPUnit assertion.
tests/ConnectionTest.php	Updates transport construction parameters.
tests/ClientFunctionalTest.php	Updates logging assertions.
tests/BulkTest.php	Threads API-version and resolver configuration.
tests/Bulk/Action/UpdateDocumentTest.php	Updates action construction.
tests/Bulk/Action/AbstractDocumentTest.php	Updates factory calls.
tests/Aggregation/GeoBoundsTest.php	Adds floating-point tolerance.
src/Transport/Http.php	Applies style normalization.
src/Transport/AwsAuthV4.php	Critical (2 votes): Guzzle 6 compatibility issue with GuzzleHttp\Utils.
src/Transport/AbstractTransport.php	Applies style normalization.
src/ServerConfiguration.php	Normalizes declarations and spacing.
src/ResultSet.php	Applies API-version and documentation cleanup.
src/RequestCounterInterface.php	Normalizes declaration formatting.
src/RequestCounter.php	Normalizes declaration and spacing.
src/Multi/MultiBuilder.php	Applies formatting cleanup.
src/Index.php	Updates formatting and API-version handling.
src/Exception/NotFoundException.php	Applies formatting cleanup.
src/ElasticSearchVersion.php	Normalizes declaration formatting.
src/Elasticsearch/Endpoints/Update.php	Normalizes function usage.
src/CustomOptions.php	Normalizes declaration formatting.
src/Connection.php	Removes an unused import.
src/Cluster/InvalidClusterConfigurationException.php	Applies formatting cleanup.
src/Cluster/ClusterConfigurationProvider.php	Applies formatting cleanup.
src/Cluster/ClusterConfigurationNotFoundException.php	Applies formatting cleanup.
src/Cluster/ClusterConfigurationFromParametersParser.php	Applies formatting cleanup.
src/Cluster/ClusterConfiguration.php	Normalizes declarations and spacing.
src/ClientFactory.php	Applies client configuration cleanup.
src/Client.php	Adds configuration, logging, and API-version handling.
src/Bulk/Response.php	Moderate (2 votes): Required API-version parameter breaks the previous public constructor arity.
src/Bulk/Action/IndexDocument.php	Removes unused imports.
src/Bulk/Action/DeleteDocument.php	Removes unused imports.
src/Bulk/Action/AbstractDocument.php	Applies API-version and formatting updates.
src/Bulk/Action.php	Removes redundant documentation.
phpstan-baseline.neon	Removes an obsolete ignore.
docker/php/Dockerfile	Moderate (2 votes): Private ECR base image blocks local builds without credentials.
composer.json	Normalizes dependency ordering.
.github/workflows/continuous-integration.yaml	Critical (3 votes): Codecov v4 lacks token/OIDC configuration. Moderate (1 vote): Local make docker-start still uses docker-compose.

Review details

Suppressed comments (1)

.github/workflows/continuous-integration.yaml:88

• This updates only the CI command, while the documented local make docker-start path still invokes docker-compose in Makefile. On the newer Docker images that no longer ship Compose v1—the reason for this change—local development remains unable to start; update the local command and its documentation as well.

          ES_VERSION=${{ matrix.elasticsearch }} docker compose --project-name=docker --file=docker/docker-compose.proxy.yml --file=docker/docker-compose.es.yml up --detach

• Files reviewed: 44/44 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jzajac-nice]

False positive for this repo: BrandEmbassy/Elastica is public, so codecov-action v4 uploads tokenless, and it defaults to fail_ci_if_error: false. The step passed on this PR (✓ Upload coverage to Codecov), so it does not hold CI red.

[jzajac-nice]

Acknowledged. In practice composer update resolves ^6.3 || ^7.2 to Guzzle 7 (where GuzzleHttp\Utils::chooseHandler() exists), and guzzle here is a dev/suggested optional transport, so CI and normal installs are unaffected. Intentionally not adding a Guzzle 6 fallback in this PR; raising the minimum to ^7.2 (Guzzle 6 is EOL) can be done separately if wanted.

[jzajac-nice]

Good catch — fixed in e004181: $apiVersion now defaults to ApiVersion::API_VERSION_7, so the public constructor keeps its old 3-arg arity for external consumers while the internal bulk path still passes the configured version.

[asyl-nice]

I get why the lint debt exists the style gate never ran against the opensearch branchs own refactor because CI was stuck queueing, so of course it surfaces a pile of unrelated violations the moment its unblocked. That part isnot really "mixed in," its a side effect you had to fix or the CS gate never goes green.
But I think there are still two genuinely different things happening here, and only one of them is trivial. On one side you've got pure reformatting with zero behavior change things like ClusterConfiguration, RequestCounter, and CustomOptions just losing blank lines between methods and switching to Yoda style comparisons and namespaced sprintf calls. Thats a rubber stamp PR on its own, nothing to actually reason about.
On the other side, Client and Bulk\Response arent style at all Client constructor signature changes (config parameter loses its array type hint, getApiVersion defaults to API_VERSION_7 instead of null), and Response gains a whole new constructor parameter. Thats real logic sitting on top of an already incomplete refactor, and its one place I actually want to slow down and read carefully which is hard to do when its buried in the same 44 file diff as the CI runner bump and two dozen whitespace only files.
So my ask is really just split along the commit boundaries you already made. CI runner modernization on its own (nothing to review, basically). The pure formatting catch up on its own (rubber stamp). And the Client/Response/ResultSet/MultiBuilder logic change with its test updates as its own PR thats the one that deserves someone's actual attention, and it shouldnt be competing for review time with a wall of whitespace diffs.

[asyl-nice]

I dont see the reason change docker-compose -> docker compose

about --project-name=docker - did you check that we need to run that jobs from docker folder only?

[asyl-nice]

same above

[chrudos-vorlicek]

Code review — NEEDS_REVISION

Reviewed at aa568ebe, diffed against origin/opensearch (eb074464, also the merge base — the branch is rebased). Read the surrounding code, the callers, the test helpers, the Makefile, the compose files and the referenced company base image, not just the diff.

3 blocking, 9 should-fix. All are inline except S6 and S9 below, which have no line in the diff to anchor to.

First, credit where it's due: this takes a branch whose pipelines never ran to fully green on 8.2 and 8.4, and the description's unusual "here's why the diff spans 40 files" section is honest enough to be checkable. So I checked it.

Is "minimal style churn" true? Yes — measured, not assumed

I ran the branch's own pinned php-cs-fixer 3.8.0 with the unmodified .php-cs-fixer.dist.php:

git checkout --detach origin/opensearch
php php-cs-fixer-3.8.0.phar fix --dry-run --allow-risky=yes --using-cache=no
#   -> exactly 24 files flagged, all under src/   (tests/ already clean)

php php-cs-fixer-3.8.0.phar fix --allow-risky=yes --using-cache=no
git add -A && TREE=$(git write-tree)
git diff $TREE aa568ebe -- src/ tests/

After normalising the base with the fixer, the entire residual delta is: src/Bulk/Response.php (disclosed), src/Client.php's semantic changes, the test fixes — and src/Transport/AwsAuthV4.php, which is the one functional change the style framing hides (M2). On this head the fixer reports zero violations, matching the green Coding style job.

So the "~24 files of pre-existing lint debt" claim is exactly right, to the file, and there is no hidden bulk restyle. Every style hunk traces to a rule in that config (native_function_invocation: {include: ['@all']} → the \sprintf/\implode churn; @Symfony → yoda + cast_spaces; phpdoc_order → @throws before @return; and so on). That part of the description is a model for how to present this kind of PR.

Blocking

M1	composer.json:18 still says php: ^7.2 || ^8.0, but the PR adds PHP-8.0-only trailing commas to parameter lists in src/Client.php — 3 on this head, 0 on the base. Unparseable on PHP 7.4, after composer says the constraint is satisfied.
M2	src/Transport/AwsAuthV4.php:20 — choose_handler() → Utils::chooseHandler() drops Guzzle 6 (absent from 6.5.8's Utils), while composer.json still allows ^6.3. Not needed, and not linter output.
M3	codecov-action@v4 upload fails on every leg (Token required - not valid tokenless upload, three times in the log) while the job reports pass.

Should fix — not anchorable inline

S6 — Makefile's docker targets still call docker-compose (v1), the very binary this PR says is gone.
docker-start, docker-stop, docker-run-phpunit, docker-run-phpcs, docker-fix-phpcs, docker-shell all invoke docker-compose. The stated reason for the CI change is that docker-compose is "gone from the new image" — CI was migrated to docker compose, but the documented local entrypoints, the ones a developer reaches for with the container this PR rewrites, were not. make docker-start fails on any machine with only Compose v2, which is every current machine. docker/docker-compose.yml:1's version: '3.4' is also obsolete under v2 and now warns — worth deleting in the same pass.

S9 — none of the new behaviour has a test. The suite is green on the old behaviour. Untested: new Client('http://host:9200') (string DSN — the entire reason the array hint came off __construct), getApiVersion() returning API_VERSION_7 by default (the change with the widest blast radius), and Bulk\Response::getApiVersion(). The first two are ~6 lines between them.

Verified clean — checked and deliberately not raised

• docker compose --project-name=docker is correct, not a magic constant. Compose v1 with -f docker/docker-compose.proxy.yml derived the project name from the first file's directory (docker), giving network docker_elastic; v2 derives it from the working directory instead. Pinning --project-name=docker is what keeps the two hardcoded --network=docker_elastic health checks working. Verified against docker/docker-compose.es.yml:46-48. Good catch on your part, and the comment explains it.
• docker/php/Dockerfile follows the current convention. I drafted a finding here and withdrew it: I had reasoned from dockerfiles/platform-backend-dev/xdebug-8.2/Dockerfile, which is a downstream image, and read its choices as evidence about its parent. The base you actually use, dockerfiles/php/8.2-debian-bullseye/Dockerfile, installs composer.phar (latest 2.x) itself — so your "composer preinstalled" comment is accurate and dropping COPY --from=composer is right — and it already ends with USER www-data, per DE-172784: non-root USER for PHP family. Both halves of my draft were wrong.
  • One thing worth a glance on first run rather than a review point: that base creates www-data at a UID that isn't 1000, and docker/docker-compose.yml:11 bind-mounts ../:/var/www/html from a host checkout that usually is. Some downstream dev images add a usermod/groupmod remap for exactly that. If composer install or vendor/ writes hit permission errors in the container, that's the fix. I couldn't test it, so it's a heads-up, not a claim.
• The phpstan-baseline.neon change raises the bar. The only edit deletes an ignore; nothing is added. reportUnmatchedIgnoredErrors is on by default, so the stale entry was itself failing the build.
• Unused-import removals are safe. Verified NullLogger has no remaining reference in src/Connection.php, and ApiVersion/Type none in src/Bulk/Action/{Delete,Index}Document.php.
• shouldLog*() returning (… & …) !== 0 is not a bug fix. src/Client.php has no declare(strict_types=1), so the old int return coerced silently — PHPStan is the reason. Harmless and clearer; the description's "real bool" just overstates it slightly.
• The remaining test edits are genuine desync fixes, not weakenings. ConnectionTest's getTransportObject(new NullLogger(), false), the buildResultSet(…, $apiVersion) / new ResultSet(…, $apiVersion) / AbstractDocument::create(…, $apiVersion, $resolver) threading, and ClientFunctionalTest's setLoggingMode(...) all match required parameters and context keys that already existed on opensearch. stringStartsWith is looser than the old exact match, but the message now interpolates an elapsed time so exact matching is impossible — a matchesRegularExpression pinning the whole format would be marginally better.
• The ECR registry passes configuration.md. 563770389081.dkr.ecr.eu-west-1.amazonaws.com embeds an AWS account id and region, both on its environment-scoped list, but this is a build-time artifact host reached identically from everywhere, which its "Accept — do not flag" list covers explicitly. No environment can leak into another through it.
• git.md — clean. Branch matches DE-XXX-descriptive-name, title matches DE-XXX Description of branch, head is rebased on its base.
• Pinning ubuntu-22.04 for PHPUnit is justified and documented; ubuntu-latest for cs/phpstan is fine, neither runs the ES cluster.
• PR size — noted, not raised. 209 added lines against php.md's ~200 SHOULD, and "one PR SHOULD keep only one idea" is stretched. But 24 of the ~40 PHP files are the pinned linter's own output and a hard prerequisite for the style gate to go green, and the rest is what makes the suite runnable at all. Not worth splitting now.
• Also considered and dropped: the new Dockerfile drops gnupg, which the old one's comment said was needed for Phive — but Makefile's tools/phive.phar target does gpg --keyserver hkps.pool.sks-keyservers.net --recv-keys, and the SKS network shut down in 2021, so make install-phpcs was already broken. Changes how it fails, not that it fails. Worth a line in the description, not a fix.

Does the title/description match the change?

Mostly, and better than most PRs of this shape. Two real gaps: AwsAuthV4.php sits in a bucket described as "mechanical style-only fixes (phpdoc, imports, whitespace)" and is a functional Guzzle API swap (M2); and "upgrade to PHP 8.2/8.4" doesn't mention that composer.json's php constraint is untouched and now false (M1).

One coordination note

opensearch does not yet contain #40. Both PRs edit src/Client.php's logSlowRequest/request region, src/Bulk/Response.php and src/Multi/MultiBuilder.php, so whichever merges second will conflict there. #40's Response::__construct work is untouched here, so the conflicts are textual rather than semantic — but worth agreeing an order rather than discovering it.

---

Standards: BrandEmbassy/developers-manifest AGENTS.md plus README.md, git.md, php.md, configuration.md, devops/README.md — all fetched successfully. Severities follow the RFC-2119 keyword of the rule cited; M1–M3 are blocking on the defect rather than on a doc keyword.

Reviewed with Claude Code.

[chrudos-vorlicek]

M1 (blocking) — this file still promises PHP 7.2, but the PR makes src/Client.php unparseable on any PHP 7.

composer.json:18 is unchanged: "php": "^7.2 || ^8.0" — in a PR titled upgrade to PHP 8.2/8.4.

Meanwhile the PR adds trailing commas to three parameter declaration lists (src/Client.php:97, :154, :181). Trailing commas in parameter declarations are PHP 8.0+ syntax — 7.3 added them for function calls only. I scanned origin/opensearch for the same construct: zero occurrences, versus exactly these three on this head. So the real floor moves from 7.4 to 8.0, and this PR is what moves it.

Failure scenario. A consumer on PHP 7.4 runs composer require. The constraint ^7.2 || ^8.0 is satisfied, so composer installs happily. The first autoload of Elastica\Client is then a fatal PHP Parse error: syntax error, unexpected ')'. The package lies about its platform requirement, and it lies at install time rather than resolve time — the one place composer cannot protect anyone.

Compounding it: the PHPUnit matrix drops 8.0 and 8.1 while ^8.0 still promises them, so two promised versions are now untested as well as one being unparseable.

Fix. Set "php": "^8.2" (what CI actually tests), drop the now-pointless symfony/polyfill-php73 (line 25), and release it as a major — a PHP-floor bump is breaking for every dependant, and branch-alias is still 7.0.x-dev. php.md § Composer: "When creating a new component version, keep in mind the versioning rules. Consult new version number with at least one other person."

[chrudos-vorlicek]

M1 (blocking), cont. — this trailing comma is PHP 8.0+ only.

Trailing commas in parameter declaration lists require PHP 8.0 (7.3 covered function calls only). Same construct at :154 and :181.

composer.json:18 still declares "php": "^7.2 || ^8.0", so on any PHP 7.x this file is now a parse error rather than a graceful failure. Verified zero such constructs on origin/opensearch and exactly three here — so this is introduced by the PR, not pre-existing.

Full reasoning and the fix on the composer.json comment.

Note these three commas are also not php-cs-fixer output — I applied the branch's pinned php-cs-fixer 3.8.0 to the base and it does not add them. They are hand edits, which is fine in itself, but they are the specific thing that raises the floor.

[chrudos-vorlicek]

M2 (blocking) — this drops Guzzle 6 support, and it sits inside the bucket the description calls "mechanical style-only fixes (phpdoc, imports, whitespace)".

Verified against Guzzle's own source:

	choose_handler()	Utils::chooseHandler()
guzzle 6.5.8	present (src/functions.php:103)	absent — src/Utils.php has only currentTime(), idnUriConvert()
guzzle 7.9.2	present (src/functions.php:59)	present

So the old call worked on 6 and 7; the new one is a fatal Error: Call to undefined method GuzzleHttp\Utils::chooseHandler() on 6. composer.json:29 still allows "guzzlehttp/guzzle": "^6.3 || ^7.2", and suggest still offers guzzle as a transport — so a consumer pinned to Guzzle 6 using AwsAuthV4 breaks at runtime.

And it was not necessary. Guzzle 7 still ships functions.php, choose_handler() emits no runtime deprecation, and PHPStan runs at level: 4 with no deprecation rules — nothing was failing.

I also confirmed this is not linter output: running the branch's own pinned php-cs-fixer 3.8.0 over origin/opensearch flags 24 files, and AwsAuthV4.php is not one of them. No fixer rule rewrites a namespaced function call into a static method call.

Fix. Either revert it, or bump the constraint to "guzzlehttp/guzzle": "^7.2" and move it into the changes section of the description. Right now it is the one functional change the style framing conceals.

[chrudos-vorlicek]

M3 (blocking) — the coverage upload now fails on every matrix leg, and the job still reports pass.

v4 replaced the bash uploader with the Codecov CLI, which requires CODECOV_TOKEN. None is supplied.

From this PR's own job log (run 34593512082, job 103243935511, PHP 8.2 — identical on the 8.4 leg):

warning -- Branch `DE-178815-ci-php82` is protected but no token was provided
error   -- Commit creating failed:  {"message":"Token required - not valid tokenless upload"}
error   -- Report creating failed:  {"message":"Token required - not valid tokenless upload"}
error   -- Upload queued for processing failed: {"message":"Token required - not valid tokenless upload"}

fail_ci_if_error defaults to false, so the step exits 0 and the check is green. Coverage has silently stopped being reported for both jobs on both PHP versions, while --coverage-clover keeps paying pcov's cost on every run and codecov.yml keeps implying a gate that no longer receives data.

A PR whose stated goal is taking the suite "from never-running / all-red to green" should not leave a gate green-but-dead — that is worse than red, because nobody looks again.

Fix. Either token: ${{ secrets.CODECOV_TOKEN }} plus fail_ci_if_error: true, or remove the step, the two --coverage-clover flags, coverage: 'pcov' and codecov.yml. Both are defensible; silently broken is not.

(Fair caveat: I can't say the v2 step worked before, because these pipelines hadn't been running. What's certain is it does not work now and does not say so.)

[chrudos-vorlicek]

S1 (should fix) — this default is silent, and it exists to make the tests pass.

The premise checks out: ClientConfiguration::$configuration (src/ClientConfiguration.php:26-41) has no apiVersion key, so getConfigValue('apiVersion') returned null against a : int return — a TypeError for anyone not going through ClientFactory.

But look at who that anyone is. tests/Base.php:51-61:

$config = ['host' => $this->_getHost(), 'port' => $this->_getPort()];
return new Client($config, $callback, $logger);

No apiVersion. Every functional test reaching $client->getApiVersion() — tests/BulkTest.php alone does so nine times — depended on that TypeError not firing. So this production default is what makes the suite green. A fail-loud production path was softened to accommodate test fixtures.

It also introduces a contradiction. ClusterConfigurationFromParametersParser.php:40 defaults the same concept the other way:

$version = $clusterConfigurationData['version'] ?? ElasticSearchVersion::VERSION_6;

Two different silent defaults for one value inside one library.

Failure scenario. A consumer constructs new Client(['host' => …, 'port' => …]) — or now new Client('http://host:9200') — against an ES6 cluster. They silently get v7 wire behaviour: no _type in bulk metadata (Bulk\Action\AbstractDocument::handleMetadataByApiVersion), no _type in mget identifiers (src/Index.php:307-318), and hits.total read as the v7 object shape rather than the v6 scalar (src/ResultSet.php:147). Wrong results and 400s instead of a loud failure at construction.

configuration.md is directly on point — apiVersion reaches ClientFactory from cluster configuration supplied per environment out of infra-ansible, and its Two traps section says of exactly this shape: "If ansible always supplies the value … the literal is latent rather than live. Still worth a should-fix, because the safety then depends on nobody forgetting." SHOULD, so non-blocking.

Fix. Pass apiVersion in tests/Base::_getClient() and keep production fail-loud — a named exception that names the missing key beats a TypeError anyway. If a default really is wanted, make it the same one the parser uses, and say so in the docblock.

[chrudos-vorlicek]

S3 (should fix) — this assertion cannot fail.

Document implements the full magic set — __get, __set, __isset, __unset (src/Document.php:53,61,66,71) — so field3 is never a real property; it lives in $_data.

property_exists($document, 'field3') is therefore false whether or not unset($document->field3) three lines earlier did anything. The test named for unsetting a field no longer verifies the unset.

To be fair: the old assertObjectNotHasAttribute was reflection-based and equally vacuous, so this is not a regression the PR introduces — but it is the moment the line is being touched, and assertObjectNotHasAttribute had to go anyway (deprecated in PHPUnit 9.6).

Fix. $this->assertFalse(isset($document->field3)); — routes through __isset — or $this->assertArrayNotHasKey('field3', $document->getData());.

[chrudos-vorlicek]

S4 (should fix) — the tolerance is relaxed 10^5x with no stated reason.

assertEquals compares floats with PHPUnit's default epsilon of 1e-10. The new delta is 1e-5 degrees ≈ 1.1 m of latitude — five orders of magnitude looser.

Elasticsearch's geo_point encoding quantizes lat/lon to ~1e-7 degrees (~1 cm), so the actual error being papered over is roughly 100x smaller than the delta chosen. As written, the assertion would also pass for a genuinely wrong bounding box a metre off.

php.md § Testing: "Deleted test SHOULD always be a red flag during Code Review process." A silently loosened one deserves the same look.

Fix. Put the observed values in the PR body and use a delta near the real error (1e-6 or tighter), with a one-line note that geo_point encoding is the cause. Same for lines 28-30.

[chrudos-vorlicek]

S7 (should fix) — ::set-output was left behind by an action-version modernization.

Three lines above this one (:63, and again at :127):

echo "::set-output name=dir::$(composer config cache-files-dir)"

::set-output is a deprecated GitHub Actions workflow command — every run logs a deprecation warning, and GitHub has announced disablement. This PR bumps checkout, cache and codecov from v2 to v4 for exactly this class of reason, then leaves the workflow command that is further along the same path.

Fix. echo "dir=$(composer config cache-files-dir)" >> "$GITHUB_OUTPUT", in both jobs.

[chrudos-vorlicek]

S8 (should fix) — no permissions: block.

The workflow declares no permissions:, so GITHUB_TOKEN gets whatever the repo/org default is. Nothing here writes — checkout, setup-php, composer, phpunit, phpstan, a coverage upload.

Fix. Add at top level, above jobs::

permissions:
  contents: read

Pinning the actions by commit SHA rather than the mutable @v4 / @v2 tags is the other half of that hardening. The PR touches all of those lines anyway, so it's a cheap moment to do it — and this repo is public, so the workflow runs on fork PRs.

[chrudos-vorlicek]

S10 (should fix) — prefer a union type over dropping the type entirely.

The widening is right — the constructor body has always handled is_string($config), so only the signature was blocking a documented feature. But with the floor now at PHP 8.x (see M1), array|string $config = [] expresses the same thing without giving up type enforcement.

As written, new Client(42) fails somewhere inside ClientConfiguration::fromArray() rather than at the boundary. php.md § Codestyle: "All code must be self-documenting."

Worth adding a unit test for the string-DSN path while you're here — it's the entire reason this type hint came off, and nothing currently exercises it (see S9 in the summary).