Improve #169: Test coverage for framework bridges by derpixler · Pull Request #175 · zammad/zammad-api-client-php

derpixler · 2026-11-02T11:02:57Z

Summary by CodeRabbit

Bug Fixes
- Invalid date strings now consistently return null on PHP 8.1 and 8.2.
- Laravel and Symfony integrations handle missing or empty credentials consistently, with errors raised when the client is requested.
New Features
- Laravel configuration now supports cached settings and gives explicitly configured values precedence over environment values.
- Framework integrations support Laravel 12 and 13, and Symfony 6.4, 7, and 8.
Documentation
- Updated integration guidance with configuration behavior and supported framework versions.

…ar (#169)

)

…169)

coderabbitai · 2026-11-02T11:03:06Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review: 1

main

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 76eff7b0-14db-45c9-bd56-1b7eb5afd1c5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

Trigger review

Walkthrough

The Laravel provider now uses packaged configuration and validates URL and token values before resolving the client. The Symfony bundle adds a default token fallback and retains lazy client resolution. Separate Composer projects add bridge tests and static analysis, with CI coverage across supported framework versions. The pull request also updates repository test commands and documentation, changes date parsing to catch Exception, and adds source archive exclusions.

Possibly related issues

zammad/zammad-api-client-php#169 — Adds bridge tests, static analysis, and CI coverage for framework bridge behavior.

Possibly related PRs

zammad/zammad-api-client-php#173 — Updates the authentication contract used by the Symfony and Laravel bridges, which this pull request also changes and tests.

Merge Risk: Moderate · previous review 5a0cc

The new bridge test targets can report success after a failure, which weakens local and pre-merge checks. One Laravel CI leg can also fail on artifact upload. Fix both before merging; the remaining findings are minor test-isolation and documentation inaccuracies.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title clearly identifies the main focus on improving test coverage for the framework bridges. This matches the added bridge tests and CI coverage, although the changes also include related behavio…

Full details: Title check

Explanation

The title clearly identifies the main focus on improving test coverage for the framework bridges. This matches the added bridge tests and CI coverage, although the changes also include related behavior fixes and documentation.

Full details: Docstring Coverage

Explanation

Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 7 functions across 6 files. (16 skipped: 16 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

CodeRabbit Verify

Run CodeRabbit Verify to test the changes in this pull request.

Recent review info

Run configuration

Source mode: Auto

Repository: Unavailable

Branch: Unavailable

File: No configuration file from this repository applied

Commit: Unavailable

Layers: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: `e199c024-7832-4780-ac25-234eedd7b00f`

Commits

Reviewing files that changed from the base of the PR and between `2d15f3d` and `5a0cc20`.

Files selected for processing: 22

`.gitattributes`

`.github/workflows/tests.yml`

`.gitignore`

`CHANGELOG.md`

`Makefile`

`README.md`

`bin/pre-commit`

`composer.json`

`phpstan.neon.dist`

`phpunit.xml.dist`

`src/Bridge/LaravelServiceProvider.php`

`src/Bridge/SymfonyBundle.php`

`src/Core/Cast.php`

`test/Bridge/Laravel/composer.json`

`test/Bridge/Laravel/phpstan.neon.dist`

`test/Bridge/Laravel/phpunit.xml.dist`

`test/Bridge/Laravel/tests/LaravelServiceProviderTest.php`

`test/Bridge/Symfony/composer.json`

`test/Bridge/Symfony/phpstan.neon.dist`

`test/Bridge/Symfony/phpunit.xml.dist`

`test/Bridge/Symfony/tests/SymfonyBundleTest.php`

`test/Unit/Core/CastTest.php`

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

coderabbitai

Fix comments: Fix CodeRabbit comments on this PR

…ked (#169)

…169)

bialaps

One test-isolation issue in the Symfony bridge suite (inline). The remaining comments are documentation accuracy.

bialaps · 2026-11-03T08:32:38Z

+        self::assertInstanceOf(ZammadClient::class, $container->get(ZammadClient::class));
+    }
+
+    public function testMissingTokenFailsWithTheLibrarysOwnMessage(): void


This test depends on the shell environment. env(ZAMMAD_TOKEN) set as a container parameter is only a fallback: Symfony reads $_ENV, $_SERVER and getenv() first, and uses the parameter only when the variable is absent (EnvVarProcessor.php L170-L171, fallback at L219-L224).

Reproduced on this branch (symfony/dependency-injection 8.1, PHP 8.5): with a clean environment all 10 tests pass; with ZAMMAD_TOKEN exported, as examples/cookbook/README.md instructs, this test fails:

1) ZammadAPIClient\Tests\Bridge\Symfony\SymfonyBundleTest::testMissingTokenFailsWithTheLibrarysOwnMessage Failed asserting that exception of type "InvalidArgumentException" is thrown.

Once the bridge projects are installed (make bridge-install), make test and bin/pre-commit run this suite, so the failure then also blocks commits.

The Laravel suite already handles this (9c5a5d6) and passes with both variables exported. Applying the same setUp()/tearDown() here (clear and restore ZAMMAD_URL/ZAMMAD_TOKEN in $_SERVER, $_ENV and via putenv()) fixes it. The class docblock (line 28, "hermetic and order-independent") needs the same correction.

bialaps · 2026-11-03T08:32:38Z

 ### Laravel

+Supported: **Laravel 12 and 13**. Each of those is exercised in CI — see the
+`unit` job in [`.github/workflows/tests.yml`](.github/workflows/tests.yml).


The Laravel legs run in the bridge-laravel job, not unit.

bialaps · 2026-11-03T08:32:38Z

 ### Symfony

+Supported: **Symfony 6.4 LTS, 7.x and 8.x**. Each of those is exercised in CI —
+see the `unit` job in [`.github/workflows/tests.yml`](.github/workflows/tests.yml).


Same here: the Symfony legs run in the bridge-symfony job, not unit.

bialaps · 2026-11-03T08:32:38Z

 `GuzzleClientFactory::withToken()` / `withOAuth2()` / `withBasicAuth()` keep their signatures and now delegate to `Auth`. They remain supported shorthand for the explicit-`Auth` constructor and are not scheduled for removal; `ZammadClient::create()` is simply the recommended entry point for new code.

-**One behavioural change to be aware of:** an empty or whitespace-only token (or username) is now rejected with an `InvalidArgumentException` at construction, where it previously produced a client that failed with HTTP 401 on its first request. Code that builds a client from an unset environment variable therefore fails earlier and more clearly than before. Both framework bridges resolve the client lazily, so this surfaces the first time something asks the container for it, not while the container is built: Laravel falls back to an empty token (`env('ZAMMAD_TOKEN', '')`) and throws `InvalidArgumentException`, while the Symfony bundle falls back to `%env(ZAMMAD_TOKEN)%` and throws `EnvNotFoundException` when the variable is unset, or `InvalidArgumentException` when it is set but empty. The Symfony bundle only became lazy in this release — see the fix above; before it, an empty token made `cache:clear` (and with it `composer install`) fail outright. The bundles are still covered by neither tests nor static analysis ([#169](https://github.com/zammad/zammad-api-client-php/issues/169)); the bundle change above was verified by compiling and dumping a container against symfony/dependency-injection 7.4 outside this repository.
+**One behavioural change to be aware of:** an empty or whitespace-only token (or username) is now rejected with an `InvalidArgumentException` at construction, where it previously produced a client that failed with HTTP 401 on its first request. Code that builds a client from an unset environment variable therefore fails earlier and more clearly than before. Both framework bridges resolve the client lazily, so this surfaces the first time something asks the container for it, not while the container is built: Laravel falls back to an empty token (`env('ZAMMAD_TOKEN', '')`) and throws `InvalidArgumentException`, while the Symfony bundle falls back to `%env(ZAMMAD_TOKEN)%` and throws `EnvNotFoundException` when the variable is unset, or `InvalidArgumentException` when it is set but empty. The Symfony bundle only became lazy in this release — see the fix above; before it, an empty token made `cache:clear` (and with it `composer install`) fail outright. The Symfony bundle is now covered by tests and static analysis in this repository, and an unset `ZAMMAD_TOKEN` no longer produces `EnvNotFoundException` — see the entries below ([#169](https://github.com/zammad/zammad-api-client-php/issues/169)).


This paragraph first says the Symfony bundle throws EnvNotFoundException for an unset ZAMMAD_TOKEN, then says it no longer does, "see the entries below". Those entries are above (the SymfonyBundle fix and the bridge test entry). Both changes ship in the same unreleased 3.2.0, so the intermediate behaviour is never visible to a user.

Suggested replacement for the Symfony part:

…while the Symfony bundle falls back to an empty token and throws InvalidArgumentException — unless config/packages/zammad.yaml sets token: '%env(ZAMMAD_TOKEN)%' explicitly, in which case Symfony raises EnvNotFoundException for an unset variable.

The trailing "and an unset ZAMMAD_TOKEN no longer produces EnvNotFoundException — see the entries below" can then be dropped.

bialaps · 2026-11-03T08:32:38Z

+resolving `ZammadClient` then throws
+`InvalidArgumentException: API token must not be empty.`
+
 For older Laravel versions, register `ZammadAPIClient\Bridge\LaravelServiceProvider::class`


Nit: "For older Laravel versions" and "auto-discovered (Laravel 5.5+)" (line 438) now sit right below "Supported: Laravel 12 and 13". Both could go, or be reworded so they don't read as supported versions.

dvuckovic

@derpixler Looks good overall: the mergeConfigFrom fix for config:cache, the empty-token default in the Symfony bundle and the wider catch in Cast all hold up. Inline comments below: three medium (Symfony test isolation, a swallowed config error in the Laravel provider, a self-contradicting CHANGELOG paragraph) and three low (README/CHANGELOG accuracy, script parity, a CI detail).

Not verified: whether the CI matrix resolves and passes (I couldn't run Composer), and the advisory ID cited in the Laravel matrix comment. That comment is also ambiguous about whether "11.x" means Laravel or PHPUnit.

dvuckovic · 2026-11-03T08:49:52Z

+     */
+    private static function setting(mixed $value, string $default): string
+    {
+        return is_string($value) && $value !== '' ? $value : $default;


Medium: a wrong-typed url silently becomes localhost. The docblock says this narrowing exists so a bad config value doesn't produce an error pointing at the client instead of config/zammad.php. But a non-string value never produces an error at all:

A non-string url (an array, an int, a wrong env() cast) becomes http://127.0.0.1:8098 without any error, and the client talks to localhost in production.

A non-string token raises "API token must not be empty.", which is misleading when the token is set but has the wrong type.

testNonStringConfigValueDoesNotReachTheClientAsTypeError locks the silent fallback in. Suggest failing with a message that names the key:

private static function setting(string $key, mixed $value, string $default): string { if ($value === null || $value === '') { return $default; } if (!is_string($value)) { throw new InvalidArgumentException(sprintf( 'config/zammad.php: "%s" must be a string, %s given.', $key, get_debug_type($value), )); } return $value; }

dvuckovic · 2026-11-03T08:51:02Z

+            "composer install -d test/Bridge/Symfony --no-interaction",
+            "composer install -d test/Bridge/Laravel --no-interaction"
+        ],
+        "test:bridge": [


Low: the bridge commands disagree. composer test:bridge runs only PHPUnit, while make test-bridge and bin/pre-commit also run PHPStan. Suggest adding the two PHPStan calls here, or pointing this script at make test-bridge, so there's one definition of "bridge checks".

dvuckovic · 2026-11-03T08:51:37Z

          extensions: xdebug
-      - run: composer install --no-interaction
-      - run: composer dump-autoload
+      - run: composer update --no-interaction


Low: composer install became composer update. Without a committed root lock this behaves the same, but it's an unexplained change now that the job runs on three PHP versions. Separately, the CHANGELOG entry says CI covers "Symfony 6.4, 7.4 and 8.x", but the matrix also has a ~7.0.0 leg, and the README says "7.x".

derpixler added 8 commits November 2, 2026 11:04

Build: keep dev tooling out of the Composer dist archive (#169)

ee6c91e

Bridge: fall back to an empty Symfony token instead of an unset env v…

9505b1f

…ar (#169)

Bridge: read the Laravel environment through the packaged config (#169)

dfb5ce5

Test: cover both bridges from isolated Composer projects (#169)

c8bd0e4

CI: give each bridge its own job across the supported versions (#169)

8ad2a5b

Docs: document the bridge test projects and the supported versions (#169

b036701

)

Test: pin the Laravel package discovery entry to the provider class (#…

4fb83d2

…169)

Cast: stop throwing on unparsable dates on PHP 8.1 and 8.2 (#169)

db8648e

derpixler changed the title ~~Test/rr/169/framework bridge coverage~~ #169 framework bridge test coverage Nov 2, 2026

derpixler changed the title ~~#169 framework bridge test coverage~~ Improve #169: Test coverage for framework bridges Nov 2, 2026

derpixler requested review from bialaps and dvuckovic November 2, 2026 11:08

derpixler self-assigned this Nov 2, 2026

coderabbitai Bot reviewed Nov 2, 2026

View reviewed changes

Comment thread .github/workflows/tests.yml Resolved

Comment thread Makefile Resolved

Comment thread README.md Resolved

Comment thread test/Bridge/Laravel/tests/LaravelServiceProviderTest.php Resolved

derpixler added 5 commits November 2, 2026 14:11

CI: stop the two Laravel 12 legs uploading the same artifact name (#169)

483751e

Make: let a failing bridge check fail the target instead of being mas…

0ff33e5

…ked (#169)

Docs: name the right exception for a missing Symfony token (#169)

5e9c000

Test: isolate the Laravel bridge tests from an exported ZAMMAD_TOKEN (#…

9c5a5d6

…169)

Docs: note that two extension methods exist only for Symfony below 8 (#…

2a924c1

…169)

derpixler mentioned this pull request Nov 2, 2026

Improve #176: raise PHPStan to 2.x and fix the type defects it surfaces #177

Merged

derpixler force-pushed the test/rr/169/framework-bridge-coverage branch from b108461 to 2a924c1 Compare November 3, 2026 07:44

derpixler force-pushed the develop branch from 2d15f3d to 2b1382d Compare November 3, 2026 07:44

bialaps reviewed Nov 3, 2026

View reviewed changes

dvuckovic approved these changes Nov 3, 2026

View reviewed changes

derpixler added 5 commits November 3, 2026 10:50

Test: isolate the Symfony bridge tests from an exported token (#169)

774ab6d

Laravel: reject a wrong-typed config value by name (#169)

e140620

Docs: the Laravel provider is auto-discovered, not hand-registered

2031fa7

Docs: point the README at the bridge CI jobs, not the unit job

da4745d

Docs: drop the changelog's self-contradicting token paragraph

2ee390c

derpixler added 3 commits November 3, 2026 10:51

Docs: list the Symfony 7.0 leg in the changelog's CI range

fb3f3c7

Composer: run PHPStan in test:bridge, like make and pre-commit

8452b24

CI: say why the unit job runs composer update, not install

a52b3eb

bialaps approved these changes Nov 3, 2026

View reviewed changes

derpixler merged commit f9eab81 into develop Nov 4, 2026
23 checks passed

coderabbitai Bot mentioned this pull request Nov 4, 2026

Develop #178

Merged

derpixler mentioned this pull request Nov 4, 2026

Fix: address the CodeRabbit findings on the bridge work (#169) #179

Merged

Conversation

derpixler commented Nov 2, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Nov 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

CodeRabbit Verify

Uh oh!

coderabbitai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bialaps left a comment

Choose a reason for hiding this comment

Uh oh!

bialaps Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvuckovic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

derpixler commented Nov 2, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Nov 2, 2026 •

edited

Loading

coderabbitai Bot left a comment •

edited

Loading

bialaps Nov 3, 2026 •

edited

Loading