Let apps declare their ability surface for docent:check - #8
Merged
Merged
Conversation
Gate::has() only reads the abilities array that Gate::define() populates. An application that bridges authorization with a single Gate::before callback defines no gates at all, so every authorize: key in its content was reported as unknown — the natural shape for any enum-backed or database-backed permission system. check.abilities accepts a list of strings or a backed-enum class-string; Docent::abilities() takes a closure for dynamic lists. A closure is deliberately not supported in config, because a closure in a config file breaks config:cache. Unset, Gate::has() remains the fallback. The same surface feeds the admin's authorize: picker, which previously had nothing to offer these applications.
…y message Review findings from an independent pass: - normalizeAbilities() cast arbitrary array entries with strval(), so [null, true] became ['', '1'] — a blank ability that would silently accept a valueless authorize: — and a nested array or object raised an opaque Array-to-string conversion instead of a usable error. Entries are now validated as non-empty strings or backed-enum cases, with the offending index and type named. Backed-enum cases passed directly are now accepted too, since Permission::cases() is the natural thing to write. - The unknown-ability message claimed "No Gate/policy defines ability X", which is false under replacement semantics: an ability can be a defined gate and still be absent from the declared surface. It is now source neutral, matching unknown-value and unknown-route. - The test asserting replacement rather than augmentation was vacuous. reports.export was never Gate-defined in the harness, so an "in_array(...) || Gate::has(...)" implementation would have passed it. It now defines the gate and asserts Gate::has() first. - abilityChecker() resolves once per site, not once per check run; a multi-site run invokes a global closure once per site. Docblock and test name corrected. - An empty declared array now has documented and tested meaning: no ability is valid, rather than falling back to Gate::has().
A ternary embedded mid-concatenation decided whether a class-string "is not an enum" or "does not exist", which is the hardest possible place to read a branch. Naming it and using interpolation matches how the rest of the package builds these messages.
…-surface # Conflicts: # CHANGELOG.md
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.
Closes #3.
The coupling
Gate::has()only reads theabilitiesarray thatGate::define()populates. An application that bridges authorization with a singleGate::beforecallback defines no gates at all, soGate::has()returns false for every one of its real permissions and everyauthorize:key in its content is reported as unknown.That's the natural shape for any enum-backed or database-backed permission system, and it currently forces the dependency the wrong way round: a docs linter driving a change to the application's authorization layer.
Pushback on the proposed mechanism
The issue suggested a closure in config:
A closure in a config file breaks
php artisan config:cache— "Your configuration files are not serializable". Documenting that as the hook would trade one footgun for a worse one, so this splits it by what each mechanism can actually hold.Config, for the static case — stays cacheable:
A service provider, for the dynamic case — where closures belong:
The enum shorthand the issue asked for is supported, since that's where the permission list lives in most apps. A class-string that isn't a backed enum raises
InvalidArgumentExceptionrather than silently yielding an empty surface.Semantics
Gate::has()rather than augmenting it. Half-declaring would make the check quietly weaker than either source alone, and there's no way to tell "I didn't list it" from "it doesn't exist".Gate::has()remains the fallback.Admin picker
Editor::pickerMeta()usedarray_keys(Gate::abilities()), so for exactly these applications theauthorize:completion list was empty. It now prefers the declared surface, falling back to the defined gates.Verification
vendor/bin/pest— 641 passed (14 new).pint --testandphpstanclean.