[FrameworkBundle] Added configuration for additionnal request formats #8944

gquemener · 2013-09-06T09:10:52Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets	#8934
License	MIT
Doc PR	~

TODO

Fix wrong xml configuration definition
Write documentation (new entry or replace symfony.com/doc/current/cookbook/request/mime_type.html ?)

hacfi · 2013-09-06T09:15:36Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

+                    ->info('request configuration')
+                    ->canBeUnset()
+                    ->children()
+                        ->arrayNode('additionnal_formats')


Should be additional_formats.

Same typo at:
https://github.com/symfony/symfony/pull/8944/files#L1R363
https://github.com/symfony/symfony/pull/8944/files#L2R19
https://github.com/symfony/symfony/pull/8944/files#L3R14
https://github.com/symfony/symfony/pull/8944/files#L4R52

Yes indeed... Sorry

hacfi · 2013-09-06T09:31:06Z

+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 · 2013-09-06T09:41:26Z

@hacfi Additional formats are not services. So using a tag would require creating hacky services.

Such bundles should prepend configurations if they want to add some formats

stof · 2013-09-06T09:43:31Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

+                ->arrayNode('request')
+                    ->info('request configuration')
+                    ->canBeUnset()
+                    ->children()


You are missing the ->fixXmlConfig('additional_format') call to make the prototyped node work properly for XML configs

stof · 2013-09-06T09:45:04Z

the XSD must be updated as well

hacfi · 2013-09-06T10:59:36Z

@stof Good to know - thanks!

gquemener · 2013-09-06T10:59:37Z

I'm wondering, does it worth it to add a setFormats method inside the listener in order to allow injecting the formats manually?

hacfi · 2013-09-06T11:05:39Z

@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 · 2013-09-06T11:18:20Z

@hacfi I was thinking of injecting the formats from a controller, but it doesn't make any sense. Let's forget the idea!

gquemener · 2013-09-06T11:20:29Z

@hacfi I took example on https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/EventListener/SessionListener.php which is a subscriber to only one event.

gquemener · 2013-09-06T12:10:12Z

@stof I've updated the xsd to allow using the following structure:

  <request>
    <additional-format name="csv">
      <mime-type>text/csv</mime-type>
    </additional-format>
  </request>

What do you think about it and does the Configuration class needs some changes, please?

stof · 2013-09-06T12:39:08Z

@gquemener The XML would not look like this but will be

<request>
    <additional-format name="csv">text/csv</additional-format>
    <additional-format name="gif">imgae/gif</additional-format>
  </request>

as it is a scalar prototype

stof · 2013-09-06T12:40:08Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

+                    ->fixXmlConfig('additional-format')
+                    ->children()
+                        ->arrayNode('additional_formats')
+                            ->prototype('array')


you are still missing the call to useAttributeAsKey. Please add a test using this feature in different formats to see that it is broken currently

Ok thanks, will do soon

lsmith77 · 2013-09-07T06:52:48Z

Btw FOSRestBundle also already provides this

stof · 2013-09-12T12:55:09Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd

+        <xsd:choice  minOccurs="1" maxOccurs="unbounded">
+            <xsd:element name="mime-type" type="xsd:string" />
+        </xsd:choice>
+        <xsd:attribute name="name" type="xsd:string" />


you should mark this attribute as required as we use it as key

Ok thanks I'll fix it.

I'm still having issue finding the correct way to support https://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 · 2013-12-22T23:20:45Z

@gquemener are you still interested in finishing this one off? Does it still make sense if FOSRestBundle already provides this feature and we've got a cookbook on how to do it with a request listener?

cordoval · 2013-12-23T05:45:31Z

src/Symfony/Bundle/FrameworkBundle/Tests/EventListener/AddRequestFormatsListenerTest.php

+    {
+        $this->assertEquals(array(
+            KernelEvents::REQUEST => 'onKernelRequest',
+        ), AddRequestFormatsListener::getSubscribedEvents());


is this indentation correct?

gquemener · 2013-12-23T12:07:51Z

@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 · 2013-12-23T12:37:33Z

is it the schema thing @gquemener ? or other part of configurations? just curious

gquemener · 2013-12-23T14:04:58Z

Yes it's the xml schema. AFAIR, I had trouble defining the additionnal formats the same way in yaml and in xml.

cordoval · 2013-12-23T14:08:00Z

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 · 2013-12-23T19:06:24Z

@gquemener romaricdrigon/MetaYaml#6 this was what i was talking about, not sure how useful

wouterj · 2013-12-24T20:36:39Z

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) { return is_array($v) && isset($v['mine_type']) })
    ->then(function ($v) { return $v['mine_type'] }
->end()

gquemener · 2013-12-24T22:15:37Z

Good to know @wouterj, I'll try asap! thanks 🎄

Conflicts: src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

gquemener · 2013-12-25T21:23:47Z

I'm closing this PR and reopening a new one, as the diff has became too huge in 3 months!

gquemener · 2013-12-25T21:42:13Z

PR is coming as soon as my "backend storage" is online

@wouterj

…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#3402 Reopening 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 replace http://symfony.com/doc/current/cookbook/request/mime_type.html ?) Commits ------- f90ba11 [FrameworkBundle] Added configuration for additionnal request formats

@wouterj

…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#3402 Reopening of symfony/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 replace http://symfony.com/doc/current/cookbook/request/mime_type.html ?) Commits ------- f90ba11 [FrameworkBundle] Added configuration for additionnal request formats

@wouterj

…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#3402 Reopening of symfony/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 replace http://symfony.com/doc/current/cookbook/request/mime_type.html ?) Commits ------- f90ba11 [FrameworkBundle] Added configuration for additionnal request formats

@wouterj

…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#3402 Reopening of symfony/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 replace http://symfony.com/doc/current/cookbook/request/mime_type.html ?) Commits ------- f90ba11 [FrameworkBundle] Added configuration for additionnal request formats

[FrameworkBundle] Added configuration for additionnal request formats

ebc22f5

hacfi reviewed Sep 6, 2013
View reviewed changes

stof reviewed Sep 6, 2013
View reviewed changes

[FrameworkBundle] Fixed mispelled "additional" word

c50d284

[FrameworkBundle] Fixed xml configuration when adding request formats

f019dec

[FrameworkBundle] Added request tag inside FrameworkBundle xsd

f92b188

stof reviewed Sep 6, 2013
View reviewed changes

gquemener closed this Sep 6, 2013

gquemener reopened this Sep 6, 2013

gquemener added 6 commits September 7, 2013 16:35

[FrameworkBundle] Removed top level id of format listener

3cab8e2

[FrameworkBundle] Fixed xml schema for embedding mime-types into format

2c5adea

[FrameworkBundle] Load the formats listener only if some are configured

5165d10

[FrameworkBundle] Added config fixtures for request key

b41388e

[FrameworkBundle] Added config fixtures for empty request formats

05ccbdb

[FrameworkBundle] Added missing php doc

f769076

stof reviewed Sep 12, 2013
View reviewed changes

cordoval reviewed Dec 23, 2013
View reviewed changes

Merge branch 'master' of github.com:symfony/symfony

5f1dbfd

Conflicts: src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

gquemener closed this Dec 25, 2013

gquemener mentioned this pull request Dec 26, 2013

[FrameworkBundle] Added configuration for additionnal request formats #9862

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle] Added configuration for additionnal request formats #8944

[FrameworkBundle] Added configuration for additionnal request formats #8944

gquemener commented Sep 6, 2013

hacfi Sep 6, 2013

gquemener Sep 6, 2013

hacfi commented Sep 6, 2013

stof commented Sep 6, 2013

stof Sep 6, 2013

stof commented Sep 6, 2013

hacfi commented Sep 6, 2013

gquemener commented Sep 6, 2013

hacfi commented Sep 6, 2013

gquemener commented Sep 6, 2013

gquemener commented Sep 6, 2013

gquemener commented Sep 6, 2013

stof commented Sep 6, 2013

stof Sep 6, 2013

gquemener Sep 6, 2013

lsmith77 commented Sep 7, 2013

stof Sep 12, 2013

gquemener Sep 12, 2013

jakzal commented Dec 22, 2013

cordoval Dec 23, 2013

gquemener commented Dec 23, 2013

cordoval commented Dec 23, 2013

gquemener commented Dec 23, 2013

cordoval commented Dec 23, 2013

cordoval commented Dec 23, 2013

wouterj commented Dec 24, 2013

gquemener commented Dec 24, 2013

gquemener commented Dec 25, 2013

gquemener commented Dec 25, 2013

[FrameworkBundle] Added configuration for additionnal request formats #8944

[FrameworkBundle] Added configuration for additionnal request formats #8944

Conversation

gquemener commented Sep 6, 2013

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacfi commented Sep 6, 2013

stof commented Sep 6, 2013

Choose a reason for hiding this comment

stof commented Sep 6, 2013

hacfi commented Sep 6, 2013

gquemener commented Sep 6, 2013

hacfi commented Sep 6, 2013

gquemener commented Sep 6, 2013

gquemener commented Sep 6, 2013

gquemener commented Sep 6, 2013

stof commented Sep 6, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsmith77 commented Sep 7, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakzal commented Dec 22, 2013

Choose a reason for hiding this comment

gquemener commented Dec 23, 2013

cordoval commented Dec 23, 2013

gquemener commented Dec 23, 2013

cordoval commented Dec 23, 2013

cordoval commented Dec 23, 2013

wouterj commented Dec 24, 2013

gquemener commented Dec 24, 2013

gquemener commented Dec 25, 2013

gquemener commented Dec 25, 2013