Skip to content

Contributors cannot verify changes to the framework bridges #169

Open

Description

@derpixler

As a contributor to the PHP API client, I want changes to the Laravel and Symfony bridges to be verifiable before they are merged, so that I can tell whether my change works instead of finding out from a user's broken application.

Use case scenarios

A green build that proves nothing

Mira sends her first pull request to the client. It touches src/Bridge/SymfonyBundle.php, changing how the client is registered with the container. She runs make test locally and pushes; CI reports success. Nothing in that result relates to her change: the bundle has no tests, and phpstan.neon.dist excludes src/Bridge/ from analysis, so neither the test run nor the static analysis ever looked at the file she edited. She has no way to find out whether the DI wiring she wrote is correct, short of building a Symfony application by hand and trying it. She expected the project's own checks to tell her.

A review with nothing to lean on

Jonas reviews that pull request. For any other file in src/ he can read the accompanying test, check that PHPStan runs at level max over it, and see the diff in coverage. For the bridge he has none of that — he is left reading the DI calls and deciding from memory whether setDefinition() with an inline Definition argument behaves the way the author assumes. He approves or blocks on judgement alone, on the two files in the project where he has the least support.

No answer to "which version does this support?"

Jonas wants to add a test for the Symfony bundle and needs to know what to test against. The suggest section of composer.json names symfony/dependency-injection and symfony/http-kernel but gives no version constraint for either, and nothing else in the repository states a supported range. The bridges are documented as a supported feature in the README and in examples/cookbook/08-symfony.php (renamed to 09-symfony.php by #171), so users reasonably assume some range is supported — but there is nothing to write a test matrix against.

Acceptance criteria

  • A change that breaks the Symfony bridge's container wiring causes CI to fail
    • Verifying the bridge does not require a running Zammad instance — it exercises DI wiring, not HTTP
  • Static analysis covers the Symfony bridge at the same level as the rest of src/
    • The excludePaths entry for it in phpstan.neon.dist is no longer needed
  • The Symfony bundle's compile-time failure on a missing token can be fixed with confidence that the replacement wiring actually works
  • The coverage report no longer counts the bridges as a gap that nobody is able to close
  • The Symfony versions the bridge supports are stated somewhere a contributor will find them
  • A follow-up issue exists for the Laravel bridge, covering the same outcomes

Background information

Both bridges and the PHPStan exclusion entered the repository in the same commit, 82881bb (the v3 refactor). The exclusion carries the comment "Bridge classes require Laravel/Symfony packages at runtime only", which explains why the packages are not required at runtime — but the packages are also absent from require-dev, and a require-dev entry is never installed for anyone who runs composer require zammad/zammad-api-client-php. The decision that keeps the dependency tree light for users is therefore not the same decision that makes the bridges untestable; the second one came along unnoticed with the first.

The result is 207 lines of public, documented API — src/Bridge/LaravelServiceProvider.php (94) and src/Bridge/SymfonyBundle.php (113) — that the project's own quality gates do not inspect, in a codebase that otherwise runs PHPStan at level max. The coverage configuration includes all of src/ with no exclusion for the bridges, so they are reported as uncovered while being impossible to cover.

This is no longer hypothetical. #171 makes Auth reject an empty credential. The Symfony bundle falls back to an empty token when ZAMMAD_TOKEN is unset, and load() runs while the container is being compiled — so an application that registers the bundle without configuring a token now fails to compile at all: every route, every console command and cache:clear included, where previously it booted and only Zammad calls returned 401.

The fix is small. Registering a lazy definition with setDefinition() and setFactory(), instead of building the instance inside load(), keeps the clearer error without taking the application down with it. It was written during that PR and discarded before being committed, for exactly the reasons above: there was no way to tell whether setDefinition() with an inline Definition argument behaves as assumed, and shipping unverified DI wiring would have broken the bundle for everyone rather than only for applications that were already misconfigured. The regression was documented in the CHANGELOG and left in place instead. This issue therefore blocks a known fix, not only future changes.

The same gap reaches beyond src/Bridge/. symfony/http-client, slim/slim and php-di/php-di are absent as well, so the custom HTTP client, Symfony and Slim recipes under examples/cookbook/ are checked by php -l and nothing else.

Possible solutions

These are ideas, not decisions — the team owns the solution.

  • Add symfony/dependency-injection and symfony/http-kernel to require-dev and drop the matching excludePaths entry. Dev dependencies do not reach users, so this costs installation time in CI and nothing else.
  • Alternatively keep them out of require-dev and add a separate CI job that installs them on demand, if keeping the default dev install small is worth the extra workflow.
  • Tests could assert the observable container behaviour: that the extension registers ZammadClient and the zammad_client alias, that resolving the service returns a working client, and that the configuration precedence (explicit config, then $_ENV, then the built-in default) holds.
  • If the bridges stay outside the test run for now, excluding them from the coverage configuration would at least stop the metric reporting a gap that cannot be acted on.
  • Version constraints could be added to the suggest entries, so the supported range is stated in the same place the packages are named.

Clarification needed

  1. Which Symfony major versions should the bridge support and be tested against? For the pending fix described in Background a single version is enough to pin the wiring down; the matrix is a separate question about what the bridge promises to support.
  2. Should the Laravel follow-up be created now as a linked issue, or only once the Symfony side is done?
  3. Should the suggest entries for the other optional packages (symfony/http-client, symfony/serializer, symfony/cache) also gain version constraints, or is that a separate concern?

Metadata

Metadata

Assignees

No one assigned

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions