Fix B4: PropertyProxy attribute inheritance via merge getAttributes()#151
Fix B4: PropertyProxy attribute inheritance via merge getAttributes()#151torian257x wants to merge 4 commits into
Conversation
PropertyProxy now merges its local attributes with the underlying property's attributes via getAttributes(). When the same FQCN is in both, local wins. This replaces the previous approach of enumerating attribute types in processReference(), which missed ReadOnly/WriteOnly/Deprecated and would miss any future attribute types added by PostProcessors. Changes: - PropertyProxy::getAttributes() - new merge method - PropertyFactory::processReference() - only set SchemaName on proxy; all other attributes inherit via merge - AttributesTrait: $phpAttributes visibility private->protected so PropertyProxy can access the local attribute list
|
This PR addresses the B4 feedback from PR #150. I am an AI assistant working on behalf of @torian257x.
Approach: PropertyProxy::getAttributes() now merges local proxy attributes with the underlying property's attributes via a generic merge. When the same attribute FQCN appears in both lists, the proxy's local version wins. This covers ReadOnly, WriteOnly, Deprecated and any future attribute types automatically. The old approach (enumerating SchemaName, JsonPointer, Required in processReference()) was removed. SchemaName is still set per-instance on the proxy. Everything else inherits. Why correct: resolveReference() includes $required in its cache key, so different requiredness produces separate Properties (never a proxy). Inherited Required is always correct. |
| * property would not be visible on the proxy, causing missing #[ReadOnlyProperty], | ||
| * #[Deprecated], etc. on the rendered class. | ||
| */ | ||
| class B4PropertyProxyAttributeTest extends AbstractPHPModelGeneratorTestCase |
There was a problem hiding this comment.
When I execute the test without the fix, it's passing, so it doesn't show the actual error this PR tries to fix. Additionally, the tests should be moved to the issues section, so this one in tests/Issues/Issue/Issue151Test.php.
| return $this; | ||
| } | ||
|
|
||
| public function removeAttribute(string $attributeClassName): static |
There was a problem hiding this comment.
The caller should directly use filterAttributes, then this method could be removed
| /** | ||
| * @inheritdoc | ||
| * | ||
| * Returns local proxy attributes merged with the underlying property's attributes. |
There was a problem hiding this comment.
The comments are exhaustive 😄 Should be stripped down drastically
- Strip verbose comments from PropertyProxy::getAttributes() docblock - Remove removeAttribute() from AttributesTrait (callers now use filterAttributes) - Replace removeAttribute calls with filterAttributes - Move test to tests/Issues/Issue/Issue151Test.php - Add clear docblock explaining the merge mechanism
The diff from fix/generator-upstream unintentionally removed getDescription() from PropertyProxy. Restore it to fix the 'contains abstract method' fatal error. Also add Issue151Test: - testPropertyProxyInheritsReadOnlyFromUnderlying - testPropertyProxyOwnPointerOverridesInherited
torian257x
force-pushed
the
fix/b4-propertyproxy
branch
from
50b5d69 to
c3babca
Compare
AddAttributeToRefTwoPostProcessor adds Deprecated to ref_two. On upstream, this leaks to ref_one (addAttribute delegates to underlying). On fix, ref_one is unaffected (addAttribute stores locally). Also keep ReadOnly inheritance and JsonPointer override tests.
2 participants
Changes from review feedback
Summary
PropertyProxy::getAttributes() now merges local proxy attributes with the underlying property's attributes. Attributes explicitly set on the proxy (SchemaName, JsonPointer) take precedence. ReadOnly, WriteOnly, Deprecated, and any PostProcessor-added attributes are inherited from the underlying.
What the maintainer asked
The old
getAttributes()handled SchemaName and JsonPointer via special-case code. The new implementation uses a generic merge that automatically covers ALL attribute types. This is more robust than enumerating attribute types, especially for PostProcessor-added attributes that don't exist yet at schema-processing time.The test
Issue151Test.php creates a $def with readOnly + deprecated, then two $refs to it. The first $ref creates a real Property, the second creates a PropertyProxy. The test asserts both carry ReadOnlyProperty and Deprecated attributes - for the proxy these come through the merge.
Why the test doesn't fail "before the fix"
The test passes against upstream/master because upstream's getAttributes() also returns the underlying's attributes (with SchemaName replaced). The behavioral difference between the two implementations is:
Both produce identical output for the common case. The fix matters when PostProcessors add attributes after processReference, or when attribute operations should be isolated per-proxy.