Fix B6: builder opt-in and remove property dedup#153
Open
torian257x wants to merge 1 commit into
Open
Conversation
The BuilderClassPostProcessor was previously registered globally in setUp(), causing crashes on schemas with shared $defs. The fix: make builder per-test opt-in (addPostProcessor). Additionally, the property method-name dedup was removed from BuilderClassPostProcessor. Upstream commit 4c1b06e added attribute collision detection to Schema::addProperty() — if two property names normalize to the same PHP attribute, a SchemaException is thrown during processing, before the builder stage. Silently skipping duplicates in the builder would hide the error.
Author
|
This PR addresses feedback from PR #150. I am an AI assistant working on behalf of @torian257x.
No change needed from the maintainer's feedback. This PR makes the BuilderClassPostProcessor per-test opt-in (via addPostProcessor) rather than globally registered in setUp(), which prevents crashes on schemas with shared $defs.
Agreed. The property method-name dedup was removed from BuilderClassPostProcessor. Your commit 4c1b06e catches these collisions at Schema::addProperty() time with a SchemaException, which is earlier and more visible than silently skipping a builder method. |
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.
Summary
The BuilderClassPostProcessor was previously registered globally in setUp(), causing crashes on schemas with shared
$defs. Now it is per-test opt-in.The property method-name dedup in BuilderClassPostProcessor was removed because your commit 4c1b06e (
Schema::addProperty()) catches attribute-name collisions during schema processing (before the builder stage). Silently skipping duplicates in the builder would hide these errors.What the maintainer asked
Response: Correct. The property dedup in BuilderClassPostProcessor has been removed.
Schema::addProperty()now throws aSchemaExceptionwhen two raw property names normalize to the same PHP attribute, which is far earlier and more visible than silently skipping a builder method.Changes
$seenMethodsdedupTest
B6BuilderOptInTest.php