Repository navigation
v5: rebuild on swagger-php's spec pipeline - #64
Open
DerManoMann wants to merge 22 commits into
Open
DerManoMann wants to merge 22 commits into
DerManoMann wants to merge 22 commits into
Conversation
Drop the openapi-extras and docblock-annotation dependency; build directly against OpenApi\Builder / Mode::SPEC and read metadata off Spec\Operation DTOs instead of the classic annotations model. - Controller-level prefix/tags/security/responses inheritance is now core swagger-php (Spec\PathItem), so openapi-extras's Controller attribute needs no replacement. Middleware gets a small in-house Attachable (Attributes\Middleware) instead — PathItem's own attachables don't clone down to operations the way tags/security/responses do, so that inheritance is resolved in OpenApiRouter itself. - Introduce RouteRegistration, a plain DTO adapters now register against instead of the raw Spec\Operation: Spec\Operation carries a live \Reflector with no __serialize(), so it can't survive the OPTION_CACHE round-trip through a PSR-16 cache. Adapters no longer depend on swagger-php at all. - Port the Laravel/Slim adapters onto the new DTO; _context is gone, replaced by getReflector()/getClassName(). - Remove the unused, unwired classic-mode VendorPropertyValidation processor, and the orphaned Lumen test fixture left behind when Lumen support was dropped in #61. - Require zircote/swagger-php ^6.9, drop openapi-extras and doctrine/annotations.
Mirrors swagger-php's own Builder (with*()/set*() returning static) rather than an untyped array keyed by string constants: - OpenApiRouter: withReload(), withCache(), withOperationIdAsName(), withLogger() replace the OPTION_* array and scan()'s bolted-on $logger param. - Adapters only ever had one option each (OPTION_AUTO_REGEX), so that's now a plain typed constructor parameter instead of an array with one key. RoutingAdapterInterface::OPTION_AUTO_REGEX is gone with it. X_NAME/X_MIDDLEWARE stay as constants — those key the open-ended `custom` map vendor-extension overrides write into, not a fixed options bag.
Tooling: - rector: was pinned at PHP_81 while composer requires >=8.2; now PHP_82 with the same prepared sets swagger-php uses (deadCode, codeQuality, codingStyle, typeDeclarations, phpunitCodeQuality) plus phpunit attribute sets, and tests/Fixtures skipped so attribute fixtures are left alone. - phpstan added at level 7 with no baseline, with an `analyse` script and its own static-analysis workflow, matching swagger-php/type-info-extras. Starting clean at 7 is affordable here precisely because v2 is a rewrite. - php-cs-fixer aligned with swagger-php: @psr12 (was the superseded @psr2), ordered_imports, php_unit_attributes, attribute_empty_parentheses, phpdoc_param_order, and crucially fully_qualified_strict_types with import_symbols — without it the FQCNs rector emits were never cleaned up. - phpunit.xml.dist was still on the PHPUnit 9 schema and emitted a deprecation every run; now 11.5 with <source>/cacheDirectory. Both frameworks move into require-dev permanently. That was impossible while Lumen was supported (it conflicted with laravel/laravel), but Lumen went in #61, so a single install now covers both adapters. Which in turn collapses the per-framework workflows: slim.yml's matrix had exactly one Slim version, so it was a plain build; only the Laravel major axis carries signal, and that folds into build.yml. Fixes found by phpstan, not invented: - parameterMetadata() ignored OpenAPI 3.1 list types, so a parameter declared `type: ['integer', 'null']` silently lost its [0-9]+ route constraint. routableType() reduces a type list to the single routable scalar. - the Laravel adapter chained ->middleware()->where(); Route::middleware() returns the middleware list when called with no arguments, so its return type is `$this|array` and the chain was unsound. lint is also decoupled from test, matching swagger-php, so `composer test` runs phpunit only and lint stands on its own.
Middleware::isRoot() now returns false. Attachable::isRoot() is unconditionally true, so a #[Middleware] with no Operation or PathItem sibling to merge into was silently collected as a root attachable and then ignored by the router — the mistake produced empty middleware lists rather than an error. Declaring it non-root makes the assembler reject the leftover instead. Attributes that merge successfully are dropped by AttributeFactory::resolveNesting() before the check, so correctly placed middleware is unaffected. Adds MiddlewareTest: merging into a method-level Operation, into a class-level PathItem, and the rejection above (verified to fail without the isRoot() override). The nesting is convention-driven with no type to enforce it, and there was no direct test of the attribute at all. Docblocks follow one shape throughout: summary ending in a full stop, a blank line, then the description. Class and method blocks stay multiline even when they only carry tags; the single-line form is reserved for a lone @var or similar.
scan() constructed its Builder inline, so every extension point swagger-php exposes — augmenters, translators, resolver, compiler, setVersion() — was unreachable. The builder is now overridable via withBuilder(), with defaultBuilder() public so a caller can start from this package's defaults and adjust rather than rebuild them. Supplying a builder means supplying it whole, logger included. Also drops the re-reporting of Result::errors()/warnings() to the logger. The builder already forwards compiler diagnostics there, so every warning was logged twice; Result carries the compiler messages only, while the logger additionally sees scan-time warnings, making it the superset and the re-report pure duplication.
scan() produced a byte-identical document to a plain Builder call, so it added nothing over using swagger-php directly. With withBuilder() and defaultBuilder() in place, customisation no longer needs it either — the one line it wrapped moves into registerRoutes(). The test helpers were its only callers, building a second OpenApiRouter purely to write an openapi.yaml that nothing reads and .gitignore excludes. Removing that halves the scan work per test app; the suite runs in roughly two thirds the time. A user wanting a document calls swagger-php's Builder or CLI, which is now the whole story rather than one of two.
tests/*/openapi.yaml was written by the test helpers, not by the router, and those writes went with scan(). The entry now excludes nothing, which is the failure mode where an exclusion keeps looking load-bearing while protecting against nothing. Stale copies removed from the working tree; confirmed a full run recreates neither.
README and Configuration.md both described 4.x: openapi-extras attributes, an options array whose every key is gone, and two worked examples that would fatal. Configuration.md also still listed the Lumen adapter removed in #61, and the Slim example used Slim 3 bootstrapping. Adds docs/Upgrading.md, leading with what PathItem gains over the Controller attribute it replaces rather than presenting a swap table, and naming the three things with no equivalent: inherit:false, the wrap envelope, and the openapi-extras attributes themselves, which cannot be kept by re-requiring the package because Mode::SPEC cannot see classic annotations. Documents generating the OpenAPI document with swagger-php directly, since this package no longer contributes to it. phpdoc filled in where the public surface is thinnest: RoutingAdapterInterface, which is what a third-party adapter implements and had no @PARAM at all, and RouteRegistration, whose reverse parameter ordering is load-bearing for Slim's nested optional placeholders. Every snippet was executed rather than composed: the README controller registers as /api/v1/getme with the operationId as its name, the CLI invocation produces the document shown, the operationId hashing workaround and the translator hook both work as written, and Middleware is confirmed absent from the output.
Every attribute except #[Middleware] is swagger-php's, so this package links rather than duplicating — a second copy would only drift. 4.x had such a link but it pointed at /guide/attributes, which no longer exists. The replacements are the spec-pipeline pages specifically: guide/spec-attributes and reference/spec-attributes cover OpenApi\Spec, whereas guide/using-attributes documents the classic OpenApi\Attributes namespace this package does not read. All three URLs verified to resolve. Also states that spec attributes are beta upstream, since the whole attribute layer this depends on carries that caveat and a user choosing the package should see it.
withBuilder(Builder) plus defaultBuilder() had two problems. "Default" was ambiguous — swagger-php's own default mode is classic, so the name suggested something this package would never use. And passing a builder silently made the constructor's $sources dead: sources went unused, zero routes registered, no warning. withBuilder(callable) fixes both. The router always builds, always applies sources, spec mode and logger, then hands the builder to the hook, which may mutate it or return a replacement. That is the shape swagger-php uses for withResolver() and withAugmenters(), and it removes a public method rather than adding one. $sources was typed string|array|Finder, which singled out Finder for no reason — it is merely one iterable — while excluding \SplFileInfo, \Reflector and every other iterable that Builder::addSource() accepts. Now matches Builder. The \Reflector case matters for reflector-driven sources.
swagger-php draws this line with no exceptions: all 28 set* methods take a value, all 7 with* methods take a callable. Converting the options array to fluent setters earlier claimed to mirror that Builder but used with* for everything, so four of five methods carried a prefix promising a hook and taking a bool or an object. setReload(), setCache(), setOperationIdAsName() and setLogger() are values. withBuilder() is the only hook and keeps its prefix. setLogger() now also matches Builder::setLogger() exactly.
Records this package's vocabulary and the words it deliberately avoids, following swagger-php's format. Their terminology is linked rather than restated; where a word belongs to both, theirs wins. The distinction the whole package rests on is operation vs route: an operation is declared, a route is registered, and this turns the first into the second — so they are never interchangeable. Also pins "middleware" to the framework's meaning rather than swagger-php's pipeline stages, and "name" to the route name rather than the operation id. Includes the set*/with* rule as a term, since getting it wrong is what prompted the file: the prefix carries meaning and a written rule catches that before the code is written rather than after. Flags one unresolved ambiguity: the rewrite is called v2 internally and the branch is v2/spec-pipeline, but it releases as 5.0.
It was in Requirements, but the introduction above it said routes come from "the attributes already describing your API" — which is wrong for most readers. Spec-pipeline adoption is close to zero, so a visitor almost certainly has classic attributes this does not read, and would have believed the package worked for them until ten lines later. Now stated once, at the top, with the beta caveat alongside it; Requirements is back to versions only.
Configuration.md held two sections that are not configuration: the
#[Middleware] attribute and the vendor extensions are what you write in
a controller, not how you set up the router. They move to the README's
"Writing the attributes" section, which already existed to cover exactly
that, and Configuration.md is now router and adapter settings only.
Both were also documented wrongly. They were described as not reaching
the OpenAPI document — the opposite is true, vendor extensions are
emitted, and that is the real tradeoff against #[Middleware], which is an
Attachable and stays out. And the declared key is unprefixed ('name'),
while the document shows x-name; the table and the example disagreed on
which spelling was which. The constants hold the unprefixed form.
Renames Upgrading.md to UpgradingTo5.md so the next major gets its own
file rather than overwriting this one.
"An error rather than a silent no-op" describes the behaviour before the change and the reasoning for making it — neither of which a reader writing a controller has any context for. The rule is what they need: it must sit beside an OA\Operation or an OA\PathItem, anywhere else raises an error. Same pattern in two more places, where "rather than" set up a contrast with something the reader never expected in the first place. The upgrade guide keeps its before/after framing, which is the one place it belongs.
Substantive: the upgrade guide said two Controller features had no equivalent and listed middlewares and inherit:false. It missed headers, which cloned shared headers onto every response in the controller and has no PathItem counterpart — declare them per response, or on a shared response component. Anyone using it would have hit silence. Also points the operation-namespacing note at swagger-php's spec attributes guide, which covers both operations and PathItem, instead of the docs root. Prose: removed the remaining clauses that justify a decision rather than state the rule — "that is the tradeoff between them", "it was bespoke", "so routing it through here would only add a layer", "the reason it is worth doing", "Composition also improved". CONTEXT.md still described attributes as "already describing an API", the same wrong assumption fixed in the README earlier. Checked mechanically: no marketing filler, no volatile counts, no unverified hedges, every backticked identifier resolves against the source, every 4.x claim verified against main, all links resolve.
swagger-php resolves a class without its own PathItem against its ancestors, which is how a base controller's prefix reaches a subclass's operations. Two things read off a PathItem did not walk that same hierarchy, so both silently applied to nothing. Middleware was looked up by the operation's exact declaring class, so a base controller carrying `#[Middleware]` contributed none — the prefix composed, the middleware did not. Path parameters were read only from the operation, but the PathItems augmenter clones tags, security and responses down to operations and deliberately not parameters, since those are emitted at path level. A placeholder declared once for the whole controller therefore reached the router with no type, pattern or optionality, which is precisely the case the upgrade guide recommends. Both now read the governing PathItem chain, outermost ancestor first. A parameter the operation declares itself still wins, keeping the position the path item gave it, because the reverse ordering the optional-parameter nesting relies on is path order. Also in passing, in the same call paths: - dedupe middleware after the `x-middleware` merge rather than before, so an attribute and a vendor extension naming the same middleware collapse - stop rewriting the route cache on a cache hit, and check the cached value is an array before trusting it - anchor the Laravel adapter's `::__invoke` strip to the end of the controller string Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ckwards `withBuilder()`, `setOperationIdAsName()` and `setLogger()` had no test at all despite being the centrepiece of the options-array-to-fluent-setters rewrite, and `CachingTest` only asserted the cache key exists — never that a hit registers what the scan did, which is the whole reason `RouteRegistration` is plain data. RouterTest covers those framework-agnostically through a recording adapter: the three setters, both `registerCached()` paths, a real PSR-16 round-trip through a serialising ArrayAdapter, the OpenAPI 3.1 type-list handling in `routableType()`, and the two hierarchy fixes. Its fixtures live outside Fixtures/Controllers so the framework tests keep a fixed route table. The caching data sets named the two cache cases the wrong way round — 'cache-reload' ran with reload off — so they asserted the opposite of what they said while still passing. Corrected, and switched to named data-set arguments: two adjacent bools are too easy to transpose positionally, and a transposed pair passes just as happily. Also drops a leftover `echo` in LaravelTest that printed a response body into the PHPUnit progress output, and gives the Slim request path its missing leading slash. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing validates that route names or paths are unique, and the two frameworks disagree about the outcome, so the README now says what each does. It is not "last one wins": both keep the *first* route scanned for a duplicate name, while a duplicate method and path loses the earlier route in Laravel and throws from FastRoute in Slim. Scan order over a directory is filesystem order, so neither outcome is worth relying on. The composer description still advertised annotations, which is exactly what 5.x stopped reading. symfony/finder is used directly by the test suite and was arriving transitively; declared now, and it still resolves at --prefer-lowest. CONTEXT.md is linked from the README as user-facing terminology but kept the internal framing an earlier commit set out to remove, naming the milestone and the branch; replaced with the release line it actually ships as. Its relationships section also now describes the PathItem chain rather than a single path item. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # .github/workflows/laravel.yml # .github/workflows/slim.yml
DerManoMann
changed the title
… it here swagger-php 6.11.0 exposes the walk as Specification::buildPathItemHierarchy(), so the private copy goes. It answered the same question — which PathItems govern an operation's class, outermost ancestor first — and two implementations of one rule drift silently: anything read off a PathItem that does not walk the chain applies to nothing, with no error to notice. Raises the swagger-php floor to ^6.11, which is free here because this branch is unreleased. Doing it after 5.0 instead would ship a private copy of a rule the library exposes and need a follow-up minor to remove it. pathParameters() and customProperties() still take a list<PathItem> and are unchanged; customProperties() still merges attachables across the chain itself, since the augmenter clones only tags, security and responses.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
openapi-router 4.x reads OpenAPI attributes through
radebatz/openapi-extrasanddoctrine/annotationson swagger-php's classic pipeline. swagger-php 6 ships the specpipeline, and most of what
openapi-extrassupplied is now core —Spec\PathItemcoversControllerand adds path-level parameters, servers, security cloning and ancestor-chaincomposition. This moves the package onto that pipeline and drops both dependencies.
Breaking. Migration steps for every change below: docs/UpgradingTo5.md.
Changes
BuilderinMode::SPEC; requirezircote/swagger-php: ^6.9.radebatz/openapi-extrasanddoctrine/annotations; docblock annotations are nolonger read.
Attributes\ControllerwithSpec\PathItem; keepAttributes\Middlewarein-house.PathItemancestorchain, which swagger-php clones only
tags,securityandresponsesfrom.withBuilder();remove
scan().RouteRegistrationDTO instead ofSpec\Operation, so registrationssurvive the PSR-16 cache.
docs/for 5.x, addCONTEXT.md.