Enforce the config guards that were declared but never checked (#222) - #242
Merged
Merged
Conversation
user, refresh_token and publishable each declared isRequired() children under a node carrying addDefaultsIfNotSet(). ArrayNode::finalizeValue() inserts the default and continues without finalising, and finalisation is where isRequired() is checked, so omitting the node skipped every guard inside it. Omitting user or refresh_token then died with a TypeError naming a Symfony internal; omitting publishable compiled clean and wired null into five services declaring string, which fails on first use. None of the four had a working configuration to preserve. The guard belongs on the array node itself, which is checked before the default is inserted. A node-level validate() only runs when the node is present, so it cannot express required presence — it is used here for the one case that is genuinely conditional, the doctrine storage handler needing an entity class.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #242 +/- ##
============================================
+ Coverage 88.39% 88.40% +0.01%
- Complexity 2664 2665 +1
============================================
Files 258 258
Lines 7718 7726 +8
============================================
+ Hits 6822 6830 +8
Misses 896 896
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 #222. Same defect as #214, in the four places it left behind.
The bug
Those contradict, and Symfony resolves it by ignoring the second.
ArrayNode::finalizeValue()inserts the default at:227andcontinues without finalising the child — and finalisation is whereisRequired()is checked. So omitting the node skips every guard inside it.The hole is exactly and only "node omitted entirely": a present-but-partial node is still validated correctly, which is why this survived.
Nothing here was a breaking change, and that was verified rather than assumed
userTypeErroratSilverbackApiComponentsExtension.php:137— container will not compilerefresh_tokenTypeErrorat:104— container will not compilepublishablenullwired into five servicesrefresh_token.options.classnullintoRefreshTokenRepositoryFor
publishable, thenullgenuinely lands:An explicitly passed
nulloverrides a constructor default, so the two that look safe are not. Every one of those throws on instantiation — in practice a 500 on any API request.So there is no working installation relying on the silent-null path, because that path does not lead anywhere that works. Both
TypeErrors are thrown from this bundle's own extension, not a dependency; nothing downstream catches any of it.The fix, and why it is not
->validate()isRequired()goes on the array node itself, whichArrayNode::finalizeValue()checks at:214— before inserting the default at:227. So a required node is enforced on omission while its children still resolve their defaults when it is present.This corrects the convention recorded after #214, which said to use a node-level
->validate()instead.validate()runs infinalize(), i.e. only when the node is present — which is precisely the case that already worked. It would have changed nothing.validate()is right for exactly one of the four:refresh_token.options.class, which is genuinely conditional on the storage handler. That key is not in the config tree at all (optionsis a free-formuseAttributeAsKeymap), so it is a mandatory setting that was never declared.Before and after
userTypeError, Symfony internal in the messageThe child config "user" under "silverback_api_components" must be configured.refresh_tokenTypeError, Symfony internalpublishableoptions.classTypeErroroptions.classrepeat_ttl_seconds = 86400)The last two rows are the guards.
isRequired()on a node could plausibly have suppressedaddDefaultsIfNotSet()on its children; it does not, and a custom storage handler is correctly unaffected.Nothing existing breaks:
SilverbackApiComponentsExtensionTest's minimal config uses a custom handler, and the functional app already setsoptions.class.Tests
7 cases added to
ConfigurationTest, written first and watched fail — 4 of 5 failed onmain, and the one that passed was the control proving a present node is still validated.PHPUnit 711 → 718. Behat unchanged at 550.
Also filed: components-web-app/docs#8 —
configuration.mdcurrently asserts the opposite of the actual behaviour, and says theuser.email_verificationbooleans are required when #214 made them optional.🤖 Generated with Claude Code