Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[FrameworkBundle] Added configuration for additionnal request formats#8944
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| ->info('request configuration') | ||
| ->canBeUnset() | ||
| ->children() | ||
| ->arrayNode('additionnal_formats') |
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.
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.
Yes indeed... Sorry
+1 AsseticBundle currently has a RequestListener which just adds additional formats. It makes sense to let bundles use this feature too. So how about adding a service tag? |
@hacfi Additional formats are not services. So using a tag would require creating hacky services. Such bundles shouldprepend configurations if they want to add some formats |
| ->arrayNode('request') | ||
| ->info('request configuration') | ||
| ->canBeUnset() | ||
| ->children() |
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.
You are missing the->fixXmlConfig('additional_format') call to make the prototyped node work properly for XML configs
the XSD must be updated as well |
@stof Good to know - thanks! |
I'm wondering, does it worth it to add a |
@gquemener What do you mean by injecting formats manually? As the listener will only use onKernelRequest I suggest to use an event listener instead of a subscriber. What do you think? |
@hacfi I was thinking of injecting the formats from a controller, but it doesn't make any sense. Let's forget the idea! |
@hacfi I took example onhttps://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/EventListener/SessionListener.php which is a subscriber to only one event. |
@stof I've updated the xsd to allow using the following structure: <request> <additional-formatname="csv"> <mime-type>text/csv</mime-type> </additional-format> </request> What do you think about it and does the |
@gquemener The XML would not look like this but will be <request> <additional-formatname="csv">text/csv</additional-format> <additional-formatname="gif">imgae/gif</additional-format> </request> as it is a scalar prototype |
| ->fixXmlConfig('additional-format') | ||
| ->children() | ||
| ->arrayNode('additional_formats') | ||
| ->prototype('array') |
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.
you are still missing the call touseAttributeAsKey. Please add a test using this feature in different formats to see that it is broken currently
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.
Ok thanks, will do soon
Btw FOSRestBundle also already provides this |
| <xsd:choice minOccurs="1" maxOccurs="unbounded"> | ||
| <xsd:element name="mime-type" type="xsd:string" /> | ||
| </xsd:choice> | ||
| <xsd:attribute name="name" type="xsd:string" /> |
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.
you should mark this attribute as required as we use it as key
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.
Ok thanks I'll fix it.
I'm still having issue finding the correct way to supporthttps://github.com/symfony/symfony/pull/8944/files#L11R131 using xml.
@docteurklein rose up the fact that it might not be possible (gquemener@b41388e#commitcomment-4065417), but the FrameworkExtensionTest architecture suggest that all configuration format should behave identically. Is it okay if it's not the case?
I mean, is it ok if it's not possible to define multiple mime-types for a given format when using xml?
@gquemener are you still interested in finishing this one off? Does it still make sense ifFOSRestBundle already provides this feature and we've gota cookbook on how to do it with a request listener? |
| { | ||
| $this->assertEquals(array( | ||
| KernelEvents::REQUEST => 'onKernelRequest', | ||
| ), AddRequestFormatsListener::getSubscribedEvents()); |
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.
is this indentation correct?
@jakzal Yes I'm still interested but I was struggling with the xml configuration definition, and didn't have time to find the correct solution. I think it'd be interesting to have this configuration directly into Symfony (as you might want to use it without using FOSRestBundle, as you don't want any rest api). Anyway, As I say, I'm hardly struggling with the xml definition, so I'm gonna take a look at how FOSRestBundle did it. |
is it the schema thing@gquemener ? or other part of configurations? just curious |
Yes it's the xml schema. AFAIR, I had trouble defining the additionnal formats the same way in yaml and in xml. |
something that may help is a library i saw the other day it would turn yml -> xml schema i think, but i may be wrong, let me check the lib and come back to you, maybe it can help but if not then so be it. 👶 |
@gquemenerromaricdrigon/MetaYaml#6 this was what i was talking about, not sure how useful |
To support your current XSD/XML, you need to add a normalizer (and I think that's perfectly valid to do): ->beforeNormalization() ->ifTrue(function ($v) {returnis_array($v) &&isset($v['mine_type']) }) ->then(function ($v) {return$v['mine_type'] }->end() |
Good to know@wouterj, I'll try asap! thanks 🎄 |
Conflicts:src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
I'm closing this PR and reopening a new one, as the diff has became too huge in 3 months! |
PR is coming as soon as my "backend storage" is online |
…equest formats (gquemener)This PR was squashed before being merged into the 2.5-dev branch (closes#9862).Discussion----------[FrameworkBundle] Added configuration for additionnal request formats| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#8934| License | MIT| Doc PR |symfony/symfony-docs#3402Reopening of#8944# TODO - [x] Fix wrong xml configuration definition (Thanks@wouterj) - [x] Change configuration key `additional_formats` to a more meaningful one - [x] Write documentation (new entry or replacehttp://symfony.com/doc/current/cookbook/request/mime_type.html ?)Commits-------f90ba11 [FrameworkBundle] Added configuration for additionnal request formats
…equest formats (gquemener)This PR was squashed before being merged into the 2.5-dev branch (closes #9862).Discussion----------[FrameworkBundle] Added configuration for additionnal request formats| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#8934| License | MIT| Doc PR |symfony/symfony-docs#3402Reopening ofsymfony/symfony#8944# TODO - [x] Fix wrong xml configuration definition (Thanks@wouterj) - [x] Change configuration key `additional_formats` to a more meaningful one - [x] Write documentation (new entry or replacehttp://symfony.com/doc/current/cookbook/request/mime_type.html ?)Commits-------f90ba11 [FrameworkBundle] Added configuration for additionnal request formats
…equest formats (gquemener)This PR was squashed before being merged into the 2.5-dev branch (closes #9862).Discussion----------[FrameworkBundle] Added configuration for additionnal request formats| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#8934| License | MIT| Doc PR |symfony/symfony-docs#3402Reopening ofsymfony/symfony#8944# TODO - [x] Fix wrong xml configuration definition (Thanks@wouterj) - [x] Change configuration key `additional_formats` to a more meaningful one - [x] Write documentation (new entry or replacehttp://symfony.com/doc/current/cookbook/request/mime_type.html ?)Commits-------f90ba11 [FrameworkBundle] Added configuration for additionnal request formats
…equest formats (gquemener)This PR was squashed before being merged into the 2.5-dev branch (closes #9862).Discussion----------[FrameworkBundle] Added configuration for additionnal request formats| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#8934| License | MIT| Doc PR |symfony/symfony-docs#3402Reopening ofsymfony/symfony#8944# TODO - [x] Fix wrong xml configuration definition (Thanks@wouterj) - [x] Change configuration key `additional_formats` to a more meaningful one - [x] Write documentation (new entry or replacehttp://symfony.com/doc/current/cookbook/request/mime_type.html ?)Commits-------f90ba11 [FrameworkBundle] Added configuration for additionnal request formats
TODO