Repository navigation
Add Auth and ZammadClient::create(), fix outdated v3 docs (#168) - #171
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review: 1
Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ials Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dvuckovic
left a comment
There was a problem hiding this comment.
@derpixler Works pretty nice, great job!
I played around with some samples, and inadvertently created a Devcontainer definition 😬 Let me know if this is something you would like to see as a separate, DX-minded improvement PR.
6d5a35e to
d44dcda
Compare
Trim credentials before validation, so a whitespace-only token is rejected like an empty one, and document that a token is required. Drop the @deprecated notices on GuzzleClientFactory::with*() and on SerializesToArray::id(). Both promised removal in v4.0, but there is no v4 milestone, issue or roadmap, and the project has no deprecation or versioning policy at all — the v2 to v3 bump removed APIs with no deprecation period whatsoever. Nor is there a reason to remove them: the helpers are four-line delegations to Auth that cost no maintenance, and id() is required by DTOInterface and called by Resource itself, so the marker contradicted a live contract and the library's own usage. ZammadClient::create() remains the recommended entry point; that is said in the class docblock, the README and the CHANGELOG instead of by a removal date the project cannot keep. Fix three defects found in review: SymfonyBundle registered the client with ContainerBuilder::set(), which stores a live instance. Instances set that way do not survive container compilation, so injecting ZammadClient failed the build with "non-existent service" — the bundle never worked in a compiled container. It now registers a lazy Definition with ZammadClient::create() as its factory and an inlined Auth definition, resolving %env(ZAMMAD_TOKEN)% at runtime so the token stays out of var/cache. Verified by compiling and dumping a container against symfony/dependency-injection 7.4 outside this repo, since src/Bridge/ has no tests and no static analysis (#169). The password parameters of Auth::basic() and withBasicAuth() are marked #[\SensitiveParameter]. An empty username throws from inside basic(), so with PHP's default zend.exception_ignore_args=0 the password appeared verbatim in the stack trace. Covered by a regression test that skips on PHP 8.1 and when the ini strips arguments. The Symfony service IDs in the custom HTTP client guide were class names rather than registered services, so copying that YAML failed the build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d44dcda to
2926f7a
Compare
Closes #168.
Why
#168 reported that the README and migration docs show
ZammadClient::withToken(), which does not exist. Looking into it, the docs were not merely stale — they described the API people reach for: both the reporter and the commenter independently expected it.The actual construction was awkward enough to explain why:
Two objects nested in one line, the factory forced on newcomers, and
with*colliding with the PSR-7 meaning of "copy with a changed property". Typingnew ZammadClient(gave no hint what belongs inside.The root cause sat deeper: authentication was Guzzle configuration. The
Authorizationheader was set as a Guzzle client default header, so anything wanting authentication had to go throughGuzzleClientFactory— which is also why the custom HTTP client guide carried a hand-writtenAuthMiddlewaredecorator for Symfony'sPsr18Client, "because it does not accept global default headers". PSR-18 has no concept of default headers at all.What changed
Authentication became a transport-independent value. The new
Authcarries the credential;RequestHandlerapplies it when composing each request, the same placeImpersonationHandleralready injectsFrom. Any PSR-18 client now authenticates identically. That section went from 41 lines to 15: the decorator is still documented, but only for the case that genuinely needs it — credentials that rotate at runtime.Authhas a private constructor, so the set of valid schemes is closed by construction rather than by convention, and an empty token or username is rejected on creation. The credential is kept out ofvar_dump()output via__debugInfo().Docs. Every
ZammadClient::withToken()example corrected. Two further defects of the same class turned up and are fixed here: the README and migration guide documentedperformOnBehalfOf()/setOnBehalfOfUser(), removed back in 3.0.0, anddocs/importedZammadAPIClient\Core\RequestHandler, which is not where that class lives. The CHANGELOG said 3.0.0 was "unreleased" and had no 3.1.0 section at all; both now carry their release dates, and the German fragments are translated.The second commit fixes the same guide further. Its Laravel override and custom-middleware examples built a
RequestHandlerwith no credentials at all, so anyone copying them got a client that failed with HTTP 401 on every request. That predates this PR — Symfony'sPsr18Clientnever set anAuthorizationheader there, and the Guzzle default header does not apply on that path — but the newauth:parameter is what made it visible. The same commit points the security notes atZammadClient::create()instead of the now-deprecated factory helpers, defines a service the Symfony YAML referenced but never declared, and settles the guide on one PSR-17 factory instead of recommending one and using another.Cookbook.
09-slim.phpcontained no Slim — it was the non-Guzzle setup. It is now07-custom-http-client.php, using the terminology Twilio, Square and PSR-18 use, and a real Slim recipe took the Slim name.00-plain.phpbecame00-connect.phpand no longer claims to be "used by recipes 01-06" (nothing includes it). Recipes are grouped, and the transport doc and recipe finally link to each other.Compatibility
Additive — targeting 3.2.0. Every v3.0/v3.1 call form still works;
GuzzleClientFactory::with*()keeps its signatures and delegates toAuth. A data-provider test pins that equivalence.The helpers are not deprecated and carry no removal date. An earlier revision marked them
@deprecated … removed in v4.0; there is no v4 milestone, issue or roadmap, and the project has no deprecation policy at all (the v2→v3 bump removed APIs with no deprecation period whatsoever), so the annotation promised a schedule nothing could keep. Nor would removal buy anything: each helper is four lines delegating toAuth. The same reasoning retired the@deprecatedmarker onSerializesToArray::id(), which shipped in 3.0.0 and 3.1.0 —DTOInterfacerequires that method andResourcecalls it three times, so it was never going anywhere either.ZammadClient::create()is steered to by the class docblock, README and CHANGELOG instead.One behavioural change worth knowing: an empty or whitespace-only token is now rejected with
InvalidArgumentExceptionat construction, where it previously produced a client that failed with HTTP 401 on its first request. Code building a client from an unset environment variable fails earlier and more clearly. Both bridges resolve the client lazily, so this surfaces on first access rather than at build time: Laravel falls back to an empty token (env('ZAMMAD_TOKEN', '')) and throwsInvalidArgumentException; Symfony falls back to%env(ZAMMAD_TOKEN)%and throwsEnvNotFoundExceptionwhen the variable is unset, orInvalidArgumentExceptionwhen it is set but empty.Verification
332 tests (24 new), phpcs, phpstan at level max. The transport-independence claim is covered by a test using the suite's plain PSR-18 recording client — not Guzzle — asserting the header actually goes out, including on the
getRaw()attachment path.src/Bridge/has neither tests nor static analysis (#169), so theSymfonyBundlechange was verified outside this repository: a throwaway project onsymfony/dependency-injection7.4 registers the extension, lets a consumer service injectZammadClient, compiles, dumps the container and boots the dumped one — which is what prod/dev cache actually runs.ServiceNotFoundExceptionat compilecomposer install)InvalidArgumentExceptionat compilezammad.yaml, env vars setzammad.yaml, no env varsEnvNotFoundExceptionon accessThe token is absent from the dumped container in every case. The
#[\SensitiveParameter]fix has a regression test that was confirmed to fail without the attribute. Not verified locally: that the attribute is inert on PHP 8.1 —composer.jsondeclares>=8.1, but only PHP 8.5 is available here and.github/workflows/tests.ymlpins 8.3 with no matrix.Every
usestatement and static call in the README anddocs/code blocks was checked against the real classes; the only remaining unresolvable ones sit in blocks explicitly labelled v2:.Changed in review
Four findings from @bialaps, all reproduced before fixing:
SymfonyBundlenever worked in a compiled container. It registered the client withContainerBuilder::set(), which stores a live instance; instances set that way do not survive compilation, so any service injectingZammadClientfailed the build with "non-existent service". This PR's new empty-token validation turned that silent breakage into a failingcache:clear, and with it a failingcomposer install. Now a lazyDefinitionwithZammadClient::create()as factory and an inlinedAuth, resolving%env(ZAMMAD_TOKEN)%at runtime so the token stays out ofvar/cache. The CHANGELOG previously justified the behaviour change by claiming 3.1 would "boot and return 401" — that never happened, and the claim is gone.Auth::basic(), so with PHP's defaultzend.exception_ignore_args=0the password was printed as a literal argument. Both password parameters now carry#[\SensitiveParameter].Psr\Http\Client\ClientInterface(Resources/config/http_client.php), andnyholm.psr7.psr17_factorycomes from the Flex recipe —nyholm/psr7itself ships no DI configuration at all, which the guide now says, with a fallback for installs without Flex.Not in this PR
#170 —
ClientInterfaceexposes onlyrepo(), marked@internal, so the contract cannot be type-hinted usefully. Changing that changes the published contract.#169 stays open. The
SymfonyBundlebug it predicted is fixed here, but the underlying gap is not:src/Bridge/still has no tests and is still excluded from PHPStan, because the repo carries neither Symfony nor Laravel as a dev dependency. The verification above was done in a throwaway project, which is a workaround, not a fix.🤖 Generated with Claude Code