-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[FrameworkBundle] Remove redundant name
attribute from default_context
#53657
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
[FrameworkBundle] Remove redundant name
attribute from default_context
#53657
Conversation
See https://symfony.com/doc/current/components/config/definition.html#array-node-options |
@smnandre Well yes, it does make sense in the example, and in some other cases such as framework:
serializer:
default_context:
-
name: one
enable_max_depth: true
yaml_indentation: 2
-
name: two
enable_max_depth: true
yaml_indentation: 2 |
As i read it you can do the following: framework:
serializer:
default_context:
foo: bar
blop: blip
name: name |
@smnandre You can still do that even with this change. |
|
@smnandre Sorry, I'm not sure what the point of your last response was. |
Let's change this on 7.1, this is not a bugfix to me. |
fa46205
to
af31d97
Compare
af31d97
to
485b756
Compare
@nicolas-grekas Done |
Thank you @HypeMC. |
@HypeMC I've spend a looong time trying to reproduce a case where those keys were lost, justifiying this call... and failed massively. Will bring concrete evidence (if it exists) before commenting next time.. Sorry for that, lesson learned. |
@smnandre No worries, better safe than sorry, thanks for investigating 😄 |
…m `default_context` (HypeMC) This PR was merged into the 7.1 branch. Discussion ---------- [FrameworkBundle] Re-remove redundant name attribute from `default_context` | Q | A | ------------- | --- | Branch? | 7.1 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT This was originally removed in #53657 but was later reintroduced in v7.1.2, likely by accident, in the following merge commit: 67ac926. Commits ------- 83fe767 [FrameworkBundle] Re-remove redundant name attribute from `default_context`
A test on documentation examples revealed that this does create a weird situation when using the config builders. The E.g. the following snippet does not work on 6.4: return static function (FrameworkConfig $framework): void {
$framework->serializer()
->defaultContext([
'allow_extra_attributes' => false,
])
;
}; And a user has to do something like: return static function (FrameworkConfig $framework): void {
$framework->serializer()
->defaultContext('can be anything as this is not used', [
'allow_extra_attributes' => false,
])
;
}; I think we can consider this a bugfix for 6.4, what do you think? |
…default_context` (HypeMC) This PR was merged into the 6.4 branch. Discussion ---------- [FrameworkBundle] Remove redundant `name` attribute from `default_context` | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes? | New feature? | no | Deprecations? | no | Issues | - | License | MIT This was initially merged into v7.1 in #53657. However, after seeing `@wouterj`'s [comment](#53657 (comment)) (which I missed earlier), I opened this PR to see if we might want to reconsider this as a bug after all. Commits ------- f71a850 [FrameworkBundle] Remove redundant `name` attribute from `default_context`
This attribute
name
doesn't exist. It's not defined in the XML schema, and it doesn't do anything.default_context
is just a simple array that's passed to the serializer.