-
Notifications
You must be signed in to change notification settings - Fork 69
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create content_on_method enhancer #171
Comments
we need to find a better name. `content_on_method` would suggest that
you add different content based on the request method.
but what you want to do, if i get it correctly, is an enhancer that
selects the controller based on the content class and the method. this
sounds like being able to have more than one controllers_by_class,
limited to methods.
the normal controllers_by_class is handled by
[FieldByClassEnhancer](https://github.com/symfony-cmf/Routing/blob/master/Enhancer/FieldByClassEnhancer.php).
we could create a MethodConditionalEnhancer that wraps another enhancer
and only triggers that if the request method is in the list of methods
that was specified.
for the bundle configuration, we would have to think of a way to add
methods to controller_by_class and have several controller_by_class.
|
That sounds like a plan.
In general this enhancer would easily add the [method]Action to a given controller as a a service key. The _content isn't touched and should be set on the usual way. So controller_by_class mapping sounds right.
Would have one problem: doing it they way described above i would expect to have controllers implementing all methods - a kind of a CRUD (Rest) all in one solution. But i think it would he better to expect a controller implementing the methods the user whants to serve. Means implement Interfaces devided by methods (RestPostAwareController, RestDeleteAwareController, ...). implenting the Controller Class and checking the interface in the enhancer is to expensive for the routing - i think. You have got any idea for that?
|
i would not do any magic. if people want magic method names, they
hopefully use FOSRestBundle which does that nicely.
i would simply add that mapper, and allow explicit configuration of
controller_by_class that is limited by method. then people can map
however they need to.
|
Doing so i would add some configuration options combined with Service definitions based on field_by_class enhancer in the bundle only? I would lose the the http method Information.
But that would mean to configure each http method i whant to Support for each content Class i what to support.
|
I could add a list of controller:[method]Action pairs per content class.
|
i would make the configuration something like controller_by_class:
My\Class:
-
methods: [put, post]
controller: service:method
-
controller: service:readMethod
Other\Class: service:method so if the value of Other\Class is not a string but an array, it needs to be a list of subsets, the last of them can omit the methods filter to match everything that remains. if you want complete automatic naming, you can use FOSRestBundle instead (and also customize methods and what not). or am i missing something that FOSRestBundle can not do that we would need with the CMF? |
That looks cool and sounds right. I just need to get the configuration to that point. BundleExtension and enhancer should be no problem.
I don't think we need FosRest for the routing. But we will do when building the Controller in the ContentBundle to get the REST support there.
|
the rest bundle is just my argument why we should not try to do a lot of
automatic model + method to controller action mapping ;-)
|
So is there a chance to create an Enhancer, which would use FosRest Magic for that mapping?
The only Routing i usualy do by FosRest is the Anotation one.
|
i have no complete overview of what FOSRest can do, but afaik its a lot
about annotations. there is quite some doc in the symfony docs:
http://symfony.com/doc/current/bundles/FOSRestBundle/index.html
i am not sure what an enhancer could do with fos rest. fos rest is
generating "static" route definitions based on naming schemes. the cmf
is mapping paths from a database and objects.
for a rest api you can either use what we discussed above with
controller_by_class per method (if you want to edit content at the seo
friendly paths) or use the FOSRest approach with resource name patterns.
how would you imagine mixing those two?
|
I currently have no idea how to merge those both behaviors. So i will implement it our way. I will start on it on my "empty" RestBundle, where i implemented the Former content_by_method too. I will separate the code on routing component and bundle afterwards, when it works. It is easier to work on component and bundle parts together for me.
|
I can help with the configuration btw :) |
Uhh nice. That sounds even better. @wouterj You have an example for me? But i think yours should be closer to the original one.
|
@ElectricMaxxx the RoutingAutoBundle does something similar, take a look at that one. You just need some normalizers. |
E.g. |
Ahh ...
I think our Solution is the wrong direction. Usually we do not map multiple methods on a single controller action. We can handle multiple ContentClasses by one action, but a POST request should be handled by a postAction() while DELETE should be done by deleteAction(). Not both methods by one. Reason is the S in SOLID and i would expect the content document as parameter for DELETE (it should exist) and not for POST (currently creating it).
So we need a mapping of multiple classes on one action and one method.
|
you can also use base classes or interfaces to group things.
would your idea be to have something like this?
```
-
controller: controller.deleteAction
methods: [DELETE]
classes:
- AppBundle\Document\Article
- AppBundle\Document\Post
- ...
```
we could still use the same enhancers if its this way round, simply
enumerating all classes to keep the enhancer simple.
|
I don't think we should allow multiple documents to be routed to the same action. It seems very strange, each document should have a dedicated HTTP entry point imo (e.g. a BlogPost::createAction() shouldn't be able to created Pages imo). My example was just to illustrate how you can do the configuration part. Given this example from @dbu: controller_by_class:
My\Class:
-
methods: [put, post]
controller: service:method
-
controller: service:readMethod
Other\Class: service:method In order for configuration to support all these, the most long version should be used as standard and all others as shortcuts. So the above example in the long version will be something like: controller_by_class:
My\Class:
-
methods: [put, post]
controller: service:method
-
method: [any] # please note that the dedicated PUT and POST methods will be handled by the previous configured controller
controller: service:readMethod
Other\Class:
-
methods: [any]
controller: service:method |
Ok. @wouterj convinced. I just whanted to avoid to much configuration. Defining each method to action mapping sounds like a step into a kind of a static routing. Other pro point is, to keep the prio configuration as well.
|
each document should have a
dedicated HTTP entry point imo (e.g. a BlogPost::createAction()
shouldn't be able to created Pages imo).
note that this will never work for creating documents. when creating, we
won't find a route and thus no content, and the mapping won't trigger.
creation is an entirely different beast, as we need to figure out which
content class to create. if we want to create by posting to the target
url which does not exist yet, we would need sort of a catch-all route
for POST, or hook into the not found event.
i actually don't see a problem to say most updates can be handled by a
generic controller, but for some documents, we need specific ones. in
createbundle, we had one controller action for every update - most of
the time, an unserializer can read the data onto the document and the
controller does not need to know about.
the essential difference of this bundle vs FOSRestBundle is that
FOSRestBundle has a URL pattern reflecting the type of document, while
here we post to the frontend URL of a document to update it.
|
Having a new (AngularJS) Version of the create stuff in mind, i musst say we need to pass the content Class as FQCN as the Property on the request. We can't POST the HTML to a endpoint at all. So we still need to wrap the content and it's properties into a resource JSON as @dantleech created on ResourcesRestBundle. So for the content class we need a user friendly SelectBox. This selection should be a hidden selection for the endpoint url too(maybe also defined in the rdf). Cause the admin has a specific collecction Point in mind, that the frontend developer does Not need to know. So by pushing some responsibility to the Client Application and doing a nice RDF will make the beast really calm.
|
@wouterj yea i would need your help with the configuration as it is one level more complicated as the one in RoutingAutoBundle. I started to implement some tests on the RestBundle, where i just implemented the old enhancer for testing it in a clean and combinded (Bundle + Component) environment. So The configuration is here the configuration i wanna inject is here or here. Simply wanted to get it into the configuration without any errors. The error is a |
I will try to work on this one on our slack day today. |
Yea ... i really would need some help with that configuration to work on. |
do you have a WIP pull request that i can use as a starting point and try to fix? |
Added some lines in my own little REST sandbox mentioned above. But will move it to bundle and component tonight.
|
@dbu i created two PRs. |
A part is done by implementing a |
To map REST routes (and other use cases) we need an enhancer for to map on controller methods based on the HTTP method.
The related discussion can be found: here There is also an example implementation mentioned or can be found here
The enhancer should be available in the bundle too.
The text was updated successfully, but these errors were encountered: