feat(schema): resolve #[BoundToOpenApiEnum] from optional enum_spec_base_path#171
Merged
Conversation
…ase_path Adds an opt-in `enum_spec_base_path` extension parameter (and matching `OpenApiSpecLoader::configure(enumBasePath: ...)` argument) used only when resolving `#[BoundToOpenApiEnum]` paths. When omitted, behaviour is bit-for- bit identical to pre-1.2.0 — single-root projects need no changes. Motivated by the bundled-external dogfood layout in issue #170: projects that point `spec_base_path` at `openapi/bundled/` (where orval-readable aggregates live) want per-enum sources under `openapi/_shared/...` to bind without writing `'../_shared/...'` in every attribute. A misconfigured `enum_spec_base_path` (parameter set but the directory does not exist) surfaces via `EnumBindingException` with the new `EnumBindingReason::EnumBasePathNotFound`, so a typo cannot silently fall through to a misleading `SpecFileNotFound` on every binding. Closes #170.
… doc accuracy) Aggregates the multi-agent review findings from PR #171: C1+C2+S2 — extension parameter hardening: - Move enum_spec_base_path read out of the spec_base_path gate so an orphaned parameter (e.g. typo'd spec_base_path) is detected as a misconfiguration instead of silently dropped. - trim() the value; treat empty / whitespace-only as absent and FATAL. - FATAL diagnostic also written to GITHUB_STEP_SUMMARY for visibility. S1 — loader configure() rejects empty enumBasePath: Throw InvalidArgumentException at the API surface, matching the allowRemoteRefs pairing checks. Prevents the unhelpful "Configured enum_spec_base_path is not a directory: " (empty after the colon) message later. I4 — type design: Add EnumBindingException::forConfig() (sibling to forScan) for global configuration failures. Switch the EnumBasePathNotFound throw to use it so enumFqcn / specPath are no longer set to an arbitrary "first failing binding" — the failure is global, not per-binding. Add new reason EnumSpecBasePathOrphaned for the parameter-validation FATALs. I1+I2+I3 — comment / doc accuracy: - Soften resolveEnumBasePath() docblock claim about "spec_base_path not configured at all" (configure() requires basePath, so the case is mostly theoretical). - Replace "pre-1.2.0 behavior" with "pre-issue-#170 behavior" — issue numbers are stable, version strings rot. - Soften README "no-op" claim to "functionally equivalent (the opt-in branch additionally validates with is_dir() before resolving any binding)". I5 — bootstrap-level integration tests: Three new cases in OpenApiCoverageExtensionEnumDriftBootstrapTest exercising auto-discovery + enum_spec_base_path round-trip: clean pass, drift detection, and orphaned-parameter FATAL. Refactor runPhpunit() helper to accept an optional spec_base_path override. I6+S3 — strengthen unit coverage: - Replace the "did it throw?"-shaped no-op equivalence test with a field-by-field comparison of detectAll() reports between the fallback branch and enumBasePath = basePath branch. - Add a trailing-slash end-to-end test through the asserter. S5 — deferred to issue #172 (CoverageMergeCommand propagation).
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
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.
Summary
Adds an opt-in
enum_spec_base_pathextension parameter (and matchingOpenApiSpecLoader::configure(enumBasePath: ...)argument) used only when resolving#[BoundToOpenApiEnum]paths. When the parameter is omitted, behaviour is bit-for-bit identical to v1.1.x — single-root projects need to change nothing.Why
Per #170:
#[BoundToOpenApiEnum]resolution is currently coupled tospec_base_path. Projects that pointspec_base_pathatopenapi/bundled/(where orval-readable aggregates live) have to write'../_shared/...'in every attribute to reach per-enum source JSONs that deliberately live outside the bundle root. The parent-traversal is awkward, fragile under bundle-layout refactors, and visually leaks the bundle directory choice into every attributed enum.Workarounds rejected for the reasons spelled out in #170 — duplicating per-enum JSONs under
bundled/re-creates the exact drift this library exists to detect, and re-pointingspec_base_pathatopenapi/would break the existing{base}/{spec}.jsonlookup.Fixes #170.
Verification
composer testpasses — 1252 tests, 3018 assertions (+10 new across the review fixup commit, on top of the original +10 from the feature commit)composer stanpasses (PHPStan level 6 clean)composer cs-checkpasses (PHP-CS-Fixer clean)End-to-end exercise:
tests/Integration/EnumDriftIntegrationTest::bundled_external_layout_resolves_via_enum_spec_base_pathand three new bootstrap-level cases inOpenApiCoverageExtensionEnumDriftBootstrapTest(auto-discovery clean, auto-discovery drift, orphaned-parameter FATAL) drive the headline dogfood layout end-to-end.Notes for reviewers
Public API surface added (intentionally additive — SemVer minor):
OpenApiSpecLoader::configure(..., ?string $enumBasePath = null)— optional 6th argument, defaults preserve all existing call sites. Empty / whitespace-only value rejected withInvalidArgumentExceptionto avoid the unhelpful "is not a directory: " (empty) diagnostic later.OpenApiSpecLoader::getEnumBasePath(): ?string— non-throwing on purpose. Absence is the documented default and the asserter relies on the null return to fall back tospec_base_path. (Contrast withgetBasePath()which throws becausespec_base_pathis functionally required for spec lookup.)OpenApiCoverageExtensionparameterenum_spec_base_path— relative paths resolve againstgetcwd(), identical tospec_base_path/output_file/sidecar_dir.trim()'d on read so XML editing artefacts (leading newlines, etc.) don't silently coerce togetcwd().EnumBindingException::forConfig()— sibling toforScanfor global configuration failures (noenumFqcn/specPathbecause the failure isn't tied to any specific binding).EnumBindingReasoncases:EnumBasePathNotFound(parameter set but path doesn't exist) andEnumSpecBasePathOrphaned(parameter set withoutspec_base_path, or empty/whitespace value).Design notes:
EnumDriftAsserter::resolveEnumBasePath()is the new private helper — it consultsgetEnumBasePath()first and only falls back togetBasePath()when the new parameter is unset.enum_spec_base_pathto the same value asspec_base_pathis functionally equivalent to omitting it (the opt-in branch additionally validates withis_dir()before resolving any binding; the fallback branch defers that check to per-filefile_exists()lookups). Pinned by a field-by-fielddetectAll()comparison test.EnumBasePathNotFound/EnumSpecBasePathOrphanedare both routed throughforConfig()— the misconfiguration is global, soenumFqcn/specPathare intentionally null instead of being set to an arbitrary "first failing binding".enum_spec_base_pathall FATAL at PHPUnit bootstrap with diagnostics also written toGITHUB_STEP_SUMMARY.oneOfenum union resolution is explicitly out of scope (called out in feat(schema): allow #[BoundToOpenApiEnum] paths to resolve from a secondary base path (or bundled-external root) #170 as a known limitation tracked from feat(schema): enum drift detection between OpenAPI spec and bound PHP enums #166).Review fixups
Commit
8376c8caddresses the multi-agent code review on the initial commit:enum_spec_base_pathno longer silently dropped (review C1, C2)forConfig()factory for global config failures (I4); docblock / README accuracy (I1, I2, I3); auto-discovery + bootstrap integration tests (I5); strengthened no-op + trailing-slash unit tests (I6, S3)enumBasePathrejected at the loader'sconfigure()API surface (S1)Follow-up
CoverageMergeCommanddoes not yet plumbenum_spec_base_paththrough toOpenApiSpecLoader::configure(). Latent today (the merge CLI does not invokeEnumDriftAsserter); worth closing alongside the next merge-CLI enum-drift feature.