Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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
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
hacfi commentedSep 6, 2013
+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? |
stof commentedSep 6, 2013
@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 |
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
stof commentedSep 6, 2013
the XSD must be updated as well |
hacfi commentedSep 6, 2013
@stof Good to know - thanks! |
gquemener commentedSep 6, 2013
I'm wondering, does it worth it to add a |
hacfi commentedSep 6, 2013
@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? |
gquemener commentedSep 6, 2013
@hacfi I was thinking of injecting the formats from a controller, but it doesn't make any sense. Let's forget the idea! |
gquemener commentedSep 6, 2013
@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. |
gquemener commentedSep 6, 2013
@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 |
stof commentedSep 6, 2013
@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 |
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
lsmith77 commentedSep 7, 2013
Btw FOSRestBundle also already provides this |
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?
jakzal commentedDec 22, 2013
@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? |
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?
gquemener commentedDec 23, 2013
@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. |
cordoval commentedDec 23, 2013
is it the schema thing@gquemener ? or other part of configurations? just curious |
gquemener commentedDec 23, 2013
Yes it's the xml schema. AFAIR, I had trouble defining the additionnal formats the same way in yaml and in xml. |
cordoval commentedDec 23, 2013
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. 👶 |
cordoval commentedDec 23, 2013
@gquemenerromaricdrigon/MetaYaml#6 this was what i was talking about, not sure how useful |
wouterj commentedDec 24, 2013
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() |
gquemener commentedDec 24, 2013
Good to know@wouterj, I'll try asap! thanks 🎄 |
Conflicts:src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
gquemener commentedDec 25, 2013
I'm closing this PR and reopening a new one, as the diff has became too huge in 3 months! |
gquemener commentedDec 25, 2013
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