[WIP] Accept header - Version support #612

amenophis · 2013-11-23T00:36:34Z

This is a first PR about my idea. It will add support for allow or not the access to methods by checking version passed into the Accept HTTP header

amenophis · 2013-11-23T00:36:54Z

@lsmith77 Do you see more what i'm doing ?

lsmith77 · 2013-11-23T08:34:29Z

Thank you for the PR. I am at a conference this weekend but should definitely have time on the train home tomorrow.

lsmith77 · 2013-11-23T08:35:30Z

related to #607

stof · 2013-11-23T11:29:16Z

DependencyInjection/Compiler/VersionPass.php

+                    'event' => 'kernel.controller',
+                    'method' => 'onKernelController',
+                    'priority' => -255
+                ))


This should be done in the DI extension, not in a compiler pass

amenophis · 2013-11-23T12:18:05Z

Hi @stof,
Thanks for your feedback !
I've done some corrections based on your remarks !

Could you review ?

stof · 2013-11-23T14:12:20Z

DependencyInjection/FOSRestExtension.php

+                    'event' => 'kernel.controller',
+                    'method' => 'onKernelController',
+                    'priority' => -255
+                ))


This should be defined in the XML file like other services, not in the PHP file

I do this in PHP as i'm not sure how to make the condition for serialization context argument. Can you say me more ?

amenophis · 2013-11-24T21:59:57Z

@stof @lsmith77 Is it better like this ?

adrienbrault · 2013-11-25T02:42:13Z

Just to make sure, with this implementation declaring 2 controllers with the same path but with different versions won't work in practice right ?

amenophis · 2013-11-25T12:36:32Z

The idea I want to adress is, I want to expose methods only for version. I want to have the same application serve multiple version of the API.

And, yes, if you declare 2 controller with an action with the same route, it do not work. But we don't need to have a same method in two controllers .

mvrhov · 2013-11-25T12:52:28Z

Sometimes there is just too much difference between two versions that having both in one controller would be impractical.

amenophis · 2013-11-25T12:54:44Z

Yes, but in this case, the method you use the Until annotation, and you declare another method with a Since annotation. If you don't want to change the name of the method, you have to switch on the version inside the main method and route into Vx or Vy method

lsmith77 · 2013-11-25T12:55:08Z

right .. this is why ideally the entire version handling would be handled inside the routing or at least be associated with the routing configuration. see also #561

amenophis · 2013-11-25T12:58:37Z

Yes, it could be better to handle the version inside the routing. Any idea on how to implement it ?

amenophis · 2013-11-25T18:14:56Z

@lsmith77 Can you explain me more ? IRC ?

lsmith77 · 2013-11-25T21:39:20Z

@amenophis sure catch me tomorrow in #symfony-dev on freenode. today i am too tired from practice.

amenophis · 2013-11-25T22:08:23Z

@lsmith77 OK ! thanks

lsmith77 · 2013-12-09T12:43:44Z

so FOSRestBundle master now can set the matched media type as a request attribute #621

amenophis · 2013-12-09T13:07:28Z

Can i do something ?

lsmith77 · 2013-12-09T13:10:10Z

EventListener/VersionListener.php

+    public function onKernelRequest(GetResponseEvent $event) {
+        $request = $event->getRequest();
+
+        $acceptHeader = $request->headers->get('Accept');


this should now be $request->attributes->get('media_type');

however it requires FOSRestBundle master with the format listener enabled.

amenophis · 2013-12-09T18:00:48Z

@lsmith77 I'm not sure about my rebase ... is it OK ?

lsmith77 · 2013-12-09T18:05:20Z

EventListener/VersionListener.php

+        return $this->version;
+    }
+
+    public function __construct(Reader $reader, $regex, Context $context = null) {


{ needs to be on a newline

amenophis · 2013-12-09T18:08:21Z

@lsmith77 Done.
Do i need to move the regex into configuration ? If yes, where ? Which key ?

lsmith77 · 2013-12-09T18:41:21Z

like discussed via IRC:

move the regexp setting to a setter method and out of the constructor
drop the context from the listener, instead set the version on the view handler (requires inject the ViewHandler params to make them dynamically settable #628)
the version listener should be enabled explicitly (off by default) like all the other listeners
add some documentation (especially that the version listener needs the format listener so that the ``media_type` is set as a request attribute

lsmith77 · 2013-12-09T22:10:46Z

Controller/FOSRestController.php

@@ -11,6 +11,7 @@

 namespace FOS\RestBundle\Controller;

+use JMS\Serializer\SerializationContext;


should be removed

lsmith77 · 2013-12-09T22:16:09Z

can you squash your commits? http://symfony.com/doc/current/contributing/code/patches.html#rework-your-patch

Toflar · 2013-12-09T22:36:26Z

The JMS Serializer has annotations for Since and Until too. Any reason why we cannot reuse them? :)

merk · 2013-12-09T22:48:17Z

Because FOSRestBundle supports the Symfony serializer as well.

Toflar · 2013-12-09T22:53:48Z

I see, thx for the hint :)

lsmith77 · 2013-12-10T07:40:43Z

EventListener/VersionListener.php

+            return;
+        }
+
+        foreach ($annotations as $annotation) {


one thing I am wondering here is how to make it possible that one could annotate multiple controllers that each cover a different version of the API for the same path.

this goes into the direction of what I wanted to achieve in https://github.com/FriendsOfSymfony/FOSRestBundle/pull/561/files#diff-3b898a9b28925d8dc60210cb06ed7719R14

lsmith77 · 2013-12-14T12:43:09Z

@amenophis I have extracted the version detection part of your PR into a new PR -> #636

This way we can add the feature quickly. For the annotations I then want to come up with a fancier solution which would work as explained in #637

[WIP] Accept header - Version support

35d9114

This is a first PR about my idea. It will add support for allow or not the access to methods by checking version passed into the Accept HTTP header

Better detection of JMS Serializer Bundle

d12e318

stof reviewed Nov 23, 2013
View reviewed changes

amenophis added 4 commits November 23, 2013 13:08

Service declaration into Extension instead of CompilerPass

a4630c0

Handle null JMS context

b7fe156

Move protected to private

b7dae5b

Use ClassUtils::getClass

92a28b7

amenophis added 2 commits November 23, 2013 13:23

Fix Typo

635ac36

Assign context to view automatically if JMS Serializer is available

8687112

stof reviewed Nov 23, 2013
View reviewed changes

amenophis added 2 commits November 23, 2013 15:14

Change null management

179510d

Move service declaration to DI configuration

fc1c8cf

lsmith77 mentioned this pull request Dec 2, 2013

refactor API to make it possible to fetch the best accept header while using format priorities willdurand/Negotiation#11

Closed

lsmith77 reviewed Dec 9, 2013
View reviewed changes

Merge remote-tracking branch 'upstream/master'

7821444

amenophis added 2 commits December 9, 2013 19:02

Use mediaType

b6bbb40

Put regex into configuration

11baf1f

lsmith77 reviewed Dec 9, 2013
View reviewed changes

Fix typos

3bc9843

amenophis added 4 commits December 9, 2013 22:31

Prepare for rebase

abfbcfc

Update

e76ef2e

Use new interface

11eff9e

Update

f44089b

lsmith77 reviewed Dec 9, 2013
View reviewed changes

Update

2768c9c

lsmith77 reviewed Dec 10, 2013
View reviewed changes

liuggio mentioned this pull request Dec 10, 2013

[WIP][RFC] yep another [Header API Versioning] recap, todo list, and PR #632

Closed

3 tasks

lsmith77 mentioned this pull request Dec 14, 2013

add VersionListener #636

Merged

lsmith77 closed this Dec 14, 2013

lsmith77 mentioned this pull request Dec 14, 2013

map multiple controllers by version for the same paths #638

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Accept header - Version support #612

[WIP] Accept header - Version support #612

amenophis commented Nov 23, 2013

amenophis commented Nov 23, 2013

lsmith77 commented Nov 23, 2013

lsmith77 commented Nov 23, 2013

stof Nov 23, 2013

amenophis commented Nov 23, 2013

stof Nov 23, 2013

amenophis Nov 23, 2013

stof Nov 23, 2013

amenophis commented Nov 24, 2013

adrienbrault commented Nov 25, 2013

amenophis commented Nov 25, 2013

mvrhov commented Nov 25, 2013

amenophis commented Nov 25, 2013

lsmith77 commented Nov 25, 2013

amenophis commented Nov 25, 2013

amenophis commented Nov 25, 2013

lsmith77 commented Nov 25, 2013

amenophis commented Nov 25, 2013

lsmith77 commented Dec 9, 2013

amenophis commented Dec 9, 2013

lsmith77 Dec 9, 2013

amenophis commented Dec 9, 2013

lsmith77 Dec 9, 2013

amenophis commented Dec 9, 2013

lsmith77 commented Dec 9, 2013

lsmith77 Dec 9, 2013

lsmith77 commented Dec 9, 2013

Toflar commented Dec 9, 2013

merk commented Dec 9, 2013

Toflar commented Dec 9, 2013

lsmith77 Dec 10, 2013

lsmith77 Dec 10, 2013

lsmith77 commented Dec 14, 2013

		@@ -11,6 +11,7 @@

		namespace FOS\RestBundle\Controller;

		use JMS\Serializer\SerializationContext;

[WIP] Accept header - Version support #612

[WIP] Accept header - Version support #612

Conversation

amenophis commented Nov 23, 2013

amenophis commented Nov 23, 2013

lsmith77 commented Nov 23, 2013

lsmith77 commented Nov 23, 2013

Choose a reason for hiding this comment

amenophis commented Nov 23, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amenophis commented Nov 24, 2013

adrienbrault commented Nov 25, 2013

amenophis commented Nov 25, 2013

mvrhov commented Nov 25, 2013

amenophis commented Nov 25, 2013

lsmith77 commented Nov 25, 2013

amenophis commented Nov 25, 2013

amenophis commented Nov 25, 2013

lsmith77 commented Nov 25, 2013

amenophis commented Nov 25, 2013

lsmith77 commented Dec 9, 2013

amenophis commented Dec 9, 2013

Choose a reason for hiding this comment

amenophis commented Dec 9, 2013

Choose a reason for hiding this comment

amenophis commented Dec 9, 2013

lsmith77 commented Dec 9, 2013

Choose a reason for hiding this comment

lsmith77 commented Dec 9, 2013

Toflar commented Dec 9, 2013

merk commented Dec 9, 2013

Toflar commented Dec 9, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsmith77 commented Dec 14, 2013