-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[FrameworkBundle] Expose configuration of PropertyAccess #11731
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
91436ce
to
2ee360b
Compare
Great! 👍 |
You need to update the XML schema definition too. |
2ee360b
to
eac64c3
Compare
@xabbuh Thanks. I update the XML schema. Can you double check, I never did that before... |
you are missing the usage of the type to define the element using it in the <xsd:element name="property-accessor" type="property_accessor" minOccurs="0" maxOccurs="1" /> |
@@ -177,4 +177,9 @@ | |||
<xsd:attribute name="debug" type="xsd:string" /> | |||
<xsd:attribute name="file-cache-dir" type="xsd:string" /> | |||
</xsd:complexType> | |||
|
|||
<xsd:complexType name="property_access"> | |||
<xsd:attribute name="magic_call" type="xsd:boolean" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attribute names should use dashes, not underscores
eac64c3
to
5bedf13
Compare
@stof Done. But other block misses |
@lyrixx nope. The issue is that what you did is not what I asked for. the definition of the your current XSD is allowing to put |
5bedf13
to
2366217
Compare
@stof Thanks. It should be ok now. I just rename |
👍 |
->getDefinition('property_accessor') | ||
->replaceArgument(0, $config['magic_call']) | ||
->replaceArgument(1, $config['throw_exception_on_invalid_index']) | ||
; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a test for this functionality to FrameworkExtensionTest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
2366217
to
75afdef
Compare
👍 |
->addDefaultsIfNotSet() | ||
->info('Property access configuration') | ||
->children() | ||
->booleanNode('magic_call')->defaultFalse()->end() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these booleanNode
children also include some ->info()
methods to explain their purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, it's a direct mapping to the class argument. So it's already documented. Then I think very few people will change values here.
Thank you @lyrixx. |
…ss (lyrixx) This PR was merged into the 2.6-dev branch. Discussion ---------- [FrameworkBundle] Expose configuration of PropertyAccess | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - ping @webmozart Commits ------- 75afdef [FrameworkBundle] Expose configuration of PropertyAccess
…ments (xabbuh) This PR was merged into the 2.6 branch. Discussion ---------- [Reference] document configurable PropertyAccessor arguments | Q | A | ------------- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#11731) | Applies to | 2.6+ | Fixed tickets | #4192 Commits ------- 828ba4d document configurable PropertyAccessor arguments
ping @webmozart