Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

add CLASS to @View annotation scopes #471

Merged
merged 1 commit into from

3 participants

@nifr

The @View annotation can be used on class level without breaking functionality.
Or is there a particular reason we have to annotate each action?

This PR widens the scope of the annotation to allow @View on Class level for all actions.

@lsmith77
Owner

it's not quite so simple I fear. toy then also need to actually read the class level annotation and decide how that works in case of overlapping definitions

@nifr

The annotation scope has been added in ef8f6cd.

This was an undocumented BC break for functionality which had been there for a while :-)

Logically the behavior should be:

  • class-level @View provides defaults for all class methods
  • method-level annotations override class-level defaults

This is how it already works out of the box with the widened scope.

Can you construct an example where this is not working?

I tested every combination that came to my mind and found all method-/class-level combinations working as described above. Only returning a FormTypeInterface overwrites class-level templateVar to 'form' for the method which is nice.

I came across this while updating an older project which was using Tag 0.11.0 of FOSRestBundle where i accidently annotated the class with @View aswell as all my methods in a controller. But didn't notice until now ..

After updating to master i faced this Exception:

[Doctrine\Common\Annotations\AnnotationException]
[Semantical Error] Annotation @FOSRest\View is not allowed to be declared on
class Vendor\MyBundle\Controller\PersonController. You may only use this
annotation on these code elements: METHOD.

Turns out just adding CLASS to the scopes and annotating a class ( not every single method ) with @View . already magically provides the expected behavior and i was able to ommit a lot of method-annotations:

  • setting a template-name on class-level @View renders it for all routes - can be useful ...
  • every xyAction's route renders Resources/views/Person/xy.twig.html correctly if no template specified on class-level but @View is present. No need to put @View on every method any more.
  • all definitions on method-level override the according class-level definitions! ( this is how it should work, right? )
  • setting a templateVar on class-level forwards the name to all methods
    • setting on method-level overrides the variable
    • returning a FormTypeInterface forwards 'form' even with class-level templateVar definition ( good behavior )
  • the request format listener works as expected i.e. person/id.json renders JSON
  • .. something missing?

A quick example - automatic route generation, implicit resource names

/**
 * @FOSRest\View(templateVar="testdata", statusCode=201)
 */
class PersonController implements ClassResourceInterface
{
    public function newAction()
    {
        return $this->formHandler->createForm();
        // template: Person/new.html.twig , template variable is 'form' , http status: 201
    }

    public function helloAction()
    {
        return "hello";
        // template: Person/hello.html.twig,  template variable 'testdata' , http status: 201
    }

    /**
     * @FOSRest\View("AnotherBundle:Person:get", templatevar="person")
     */
    public function getAction(Person $person)
    {
        return $person;
       // template: AnotherBundle:Person:get , template variable is 'person' , http status: 201
    }

    /**
     * @FOSRest\View("AnotherBundle:Person:overview", templatevar="persons", statusCode=200)
     */
    public function cgetAction()
    {
        return $this->personManager->findAll();
       // template: AnotherBundle:Person:overview , template variable is 'persons' , http status: 200
    }

    // ...

This can potentially really slim down controllers!

@nifr

What needs to be done to get this merged ? Please provide some information.
Some functional tests?

btw Travis build failed due to Composer being older than 30days - no unit tests failing.

This is not only a great enhancement in terms of slim controllers and DRY - it has already quietly been working before 0.11 :)

@lsmith77
Owner

ok .. i wasnt aware that things worked like this.

@lsmith77
Owner

can you add a test case for this so that it doesnt get broken again and a doc example?

@lsmith77
Owner

ping

@nifr

will do - currently on vacaction, gonna tackle it next week! :)

@lsmith77
Owner

ping :)

@nifr

back in the game :)

maybe we should discuss wether scope-widening more annotations would make sense.

i.e. controller's having more @NoRoute methods than @Route methods could benefit.

I'm not sure wether the http method annotations ( @Get, @Put , etc... ) on class-level could make sense in some rare cases aswell. Imagine a controller handling only post requests i.e.

What's your oppinion?

@lsmith77
Owner

If it works, then I guess its ok to widen the scope and if we do, then ideally we do it for an entire "group" of annotations and not just for some.

@stof
Owner

I don't think NoRoute makes sense at the controller level. If you don't want any route on the controller, don't import it as type rest

@lsmith77
Owner

I think the point is whitelisting vs. blacklisting.

@nifr

@stof adding NoRoute to a class having i.e. only one or two routes to be exposed but a large number of methods to be excluded would prevent having to add NoRoute to all of these methods.

I think this would come in handy sometimes.

general question - how should we test the class-level annotation behavior most intelligently? suggestions?

@lsmith77
Owner

@nifr well you just need to add relevant classes to the Fixtures. btw class level @NoRoute only makes sense if its possible to override this with a method level annotation.

@lsmith77
Owner

btw .. i would like to tag 0.13.0 ASAP .. so it would be great to wrap this up.

@lsmith77
Owner

ping

@lsmith77
Owner

ping

@lsmith77 lsmith77 merged commit d6c72b6 into FriendsOfSymfony:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 6, 2013
  1. added CLASS to @View annotation scopes

    Nicolai Froehlich authored
This page is out of date. Refresh to see the latest.
Showing with 1 addition and 1 deletion.
  1. +1 −1  Controller/Annotations/View.php
View
2  Controller/Annotations/View.php
@@ -16,7 +16,7 @@
/**
* View annotation class.
* @Annotation
- * @Target("METHOD")
+ * @Target({"METHOD","CLASS"})
*/
class View extends Template
{
Something went wrong with that request. Please try again.