symfony-cmf / content-bundle

Provides some basic document classes and controllers for modeling content
https://cmf.symfony.com
19 stars 32 forks source link

REST write support #93

Open lsmith77 opened 10 years ago

lsmith77 commented 10 years ago

with https://github.com/symfony-cmf/ContentBundle/pull/91 and https://github.com/schmittjoh/serializer/pull/184 REST read support is basically available.

however it would be cool to get some more sane defaults for the response and/or more aspects that can be configured globally without having to map it out for each class. furthermore we need some handling for relations so that they can optionally just be referenced by url/id rather then be inlined.

dbu commented 10 years ago

@lsmith77 you did some tweaking, right? what is still missing?

lsmith77 commented 10 years ago

we have read support now .. but no write support

lsmith77 commented 10 years ago

maybe this is better delegated to SyliusResourceBundle or we extend it for our needs. need a bit of work however on some details however: https://github.com/Sylius/Sylius/issues/2057

dbu commented 10 years ago

i agree, this should not be specific to the content bundle. note that CmfCreateBundle also has a generic write handler, for json-ld, and tied to the createphp mapping rules.

lsmith77 commented 10 years ago

another possibility http://stanlemon.net/2014/10/07/demoing-lemonrestbundle-with-ng-admin/

ElectricMaxxx commented 9 years ago

@lsmith77 You have got some more ideas with that issue? Would like to work on it on our slacktime day on friday.

lsmith77 commented 9 years ago

not really .. basically the controller will need to get some more methods for POST (adding a new page), PUT (updating a page) and DELETE (deleting a page).

Note that a page obviously consists of 1) a route and 2) a content document (with references and children). I think for a DELETE it should be possible to only delete the route and not the content. Either way both the route and the content can have children, especially in this case we might have to do some cleanups to ensure we do not end up with routes that point to non existent content. Maybe eventually we also want to support some modes for DELETE where for example the node is not deleted if there are children, but instead just the data in the node is removed. But this is all future stuff ..

ElectricMaxxx commented 9 years ago

In generall controller would extend or replace the existing ContentController(https://github.com/symfony-cmf/ContentBundle/blob/master/Controller/ContentController.php) one and should be activated by a config?

What about access? OPTION should answer some allowed methods i think. What about Content that isn't rendered by the ContentController? Cause the ContentController would work for template_by_class and template_by_type only atm, right? So there should be some re-useable code for custom controllers, i think.

lsmith77 commented 9 years ago

we could look into moving some logic to a helper yes .. but imho that is a 2nd step. until then people can also extend the default controller to add functionality of needed.

lsmith77 commented 9 years ago

something like https://github.com/Sylius/Sylius/blob/master/src/Sylius/Bundle/ResourceBundle/Controller/DomainManager.php could be useful here ..

maybe this can be "unbundled" from ResourceBundle

ElectricMaxxx commented 9 years ago

ok, had a look at their ResourceController too. Would do same, but without their container injection. :-) You mean the event mechanism should be implemented, too?

lsmith77 commented 9 years ago

maybe .. my main point was that it could be useful to have a separate class to handle the actual interaction with the EM/DM

dbu commented 9 years ago

just from the name and idea, this reminds me of https://github.com/sonata-project/SonataAdminBundle/blob/master/Model/ModelManagerInterface.php which is the sonata bridge thing to abstract from doctrine.

i guess extracting something like this into a standalone thing could be useful. sonata is doing way too much for us (its aimed at the admin lists and similar things, with paging and whatnot) while the sylius one seems rather limited (e.g. the "move" is some undocumented numeric value that seems to be added to the relative position of a model).

i am not sure if it makes sense to share this abstraction with sylius or sonata, or if its better we do our own that can do the operations that we need. our own would duplicate some logic, but extracting some other thing could be a major undertaking and we would need a common agreement on what is needed. it does sound like a useful endavour, but could be changed as a version 2.

ElectricMaxxx commented 8 years ago

I am back on that issue again and still have some questions. Please just correct me.

  1. A POST of an existing content to an other route would be a new route for that content.
  2. A PUT of an existing content to an other would be a move action. Changes in content should be persisted too.
  3. A DELETE of an existing content would be just a delete of that route.
  4. POST of new Content should be restricted on specific collections. The route will be created with the contents local name as its own local name.
  5. PUT of a new content on a specific Route would create that content with that route.
  6. a GET request of an existing content in one route would serve other routes of that content in header (Seo can help)
dbu commented 8 years ago

those semantics sound right to me.

i wonder how this should or should not interact with ResourceRestBundle and/or RoutingAutoBundle? though RoutingAuto is different - here we would have the desired URL and don't need any guesser to figure out the URL, only the correct document class.

how do you know what type of content document to create here? is the client supposed to send that in meta data? CreateBundle is doing something similar already with the json-ld data and a mapping from types to document classes.

i think such mappings and similar logic sounds like it should be in ResourceRestBundle.

lsmith77 commented 8 years ago

there is a big difference with ContentBundle uses the routing to find documents, while ResourceBundle goes directly to the content structure.

ElectricMaxxx commented 8 years ago

Yea exactly @lsmith and so the the ContentBundle's REST API would be the right endpiont for native Frontend editing as a user would like to edit content where they find it in routing.

The API build by ResourcesRestBundle would be a nice endpoint for an Client side admin application. (Example)

dbu commented 8 years ago

okay. makes sense. i am just a bit afraid of ending up with 4 partly working editing solutions (sonata, create, resource rest, content rest)

ElectricMaxxx commented 8 years ago

@dbu I see create = content rest or as an replacement for the CreateBundle you where talking yesterday on twitter.

ElectricMaxxx commented 8 years ago

@lsmith77 & @dbu i still have some problems thinking on the routing: atm the GET routes, which are rendered by ContentController::indexAction() are done by the dynamic router, right? So what about:

1.) enabling other actions on routes the dynamic router knows, like PUT & DELETE 2.) enabling routes to POST or PUT (move) to, when the dynamic routing does not know them

PUT would mean there must be a known content document, so it would be like removeRoute/addRoute(). POST would mean to devide into known content - means adding route - and unknown content which would mean to create both - content and route. But on both situation we need to train the route to route those actions to the ContentController::postAction() / 'ContentController::putAction()`. Same for DELETE

wouterj commented 8 years ago

Won't adding ContentBundle REST support mean doing CreatePHP without RDFa, meaning a less generic solution than the current state?

Create stuff is a bit unmaintained (I've plans for revamping the CmfCreateBundle btw), so we may have to create a new library that replaces the Create packages. However, I think we should still use some kind of RDFa mapping or the like. Don't forget that the documents in this bundle are just meant as a base and almost every application will extend them.

Btw, for the route generation, I think using RoutingAuto and it's multi route support would be a good solution.

ElectricMaxxx commented 8 years ago

@WouterJ simply no :-)

I think we can't remove the RDFa. We do need them for the Frontend editing. So they should come with some metadata or on an specific api in the ResourcesRestBundle (my oppinion). So the Create stuff should be devided into four parts:

dbu commented 8 years ago

i am not sure how much we can reverse the routing information. we have content class to controller mappings. this can not easily be reversed as there could be several classes going to the same controller. we could kindof build a similar thing but for POST, where a map of class to controller tells which creator method to use. but i think wouter is correct that this is redoing what RestBundle is supposed to do, and what CreateBundle already does.

i think the only thing not covered in the current RestBundle / CreateBundle discussions is how the front-end can define the target URL (aka route) of a new content. but i am not sure if it makes sense to have a third option besides a REST api that allows to create the route and then create the document, and the routing auto bundle that allows to autogenerate the route.

ElectricMaxxx commented 8 years ago

So ...

I think i would have a first suggestion: RestBundle Just started working on a first issue. By the help of the routing bundle and an own enhancer a controller given as a pure service name will get its method depending on the method. I tested it with the given methods. And yea ... i can route each method to a specific method in a dummy controller.

The ideas are:

Note: there is just one test, which helped me to implement the example. There can be some stuff copied from routing-bundle (g) and there is no stuff like xsd. I will add/fix that later on. I just wanted to get feeling for that.

dbu commented 8 years ago

i don't want to be negative, but is that RestBundle doing anything that FOSRestBundle does not offer? i think i would rather follow this approach:

ElectricMaxxx commented 8 years ago

Yea it was my fault. Did the comment on the wrong issue. @dbu you are right, yes. But only on a half way. I wanted to do the comment on this issue So the ideas should go there. And now on the things, you are right:

FOSRest should do the work for the ContententBundle. I would suggest a so called RESTContentController as a default one like the generic ContentController. That controller can handle the requests and do the output by the help of FosRest. But what about the routing? I don't think that FosRest can do our dynamic routing. That would be the purpose of the bundle suggested in my comment. I should call it `RestRoutingBundle. Just as a little addon to our routing to match REST routes (meas with methods) on the routes defined by the contents. The error handling including generic error message creation can be a nice feature of the RestRoutingBundle. Thinking on the rdf-mapping i would see them provided by the RestResourcesBundle as they really a very special resources.

Doing so we would keep the responsibilities in each bundle low. It would be a bad idea to implement routing stuff inside the ContentBundle.

wouterj commented 8 years ago

REST routes are directly related to a Controller, right? So those routes are as static as they can get, aren't they? I fail to see the exact benefit of using the dynamic router here.

ElectricMaxxx commented 8 years ago

@WouterJ for the ContentBundle we wanna have routes which are exactly the same routes as the content is currently available on GET.

Example: Content: /cms/content/some/content got a Route /cms/routes/some/collection/content so it is available under example.de/some/collection/content (with the right configuration) Thats the way we currently use it. The dynamic router and its helper matches the route to some controller (by enhancer and so on) and add the content document to the request's parameter bag.

What we whant for ContentBundle's REST functions are routes like:

  1. GET /some/collection/content (same behavior)
  2. PUT /some/collection/content - update that content
  3. DELETE /some/collection/content - delete that route, we won't delete the content
  4. PUT /some/other-collection - move the content
  5. POST /some/collection - create a new content

For 1-3 the dynamic router will do the work as it does it now, it just matches the route, adds the content. But we do not have any controller_by_method enhancer to get a specific action and the new Bundle would simply add a method aware enhancer (and i still have one of those enhancers in the bundle working) For 5 and 6 we do need some collection and points and again simply an enhancer which adds the right controller:method combination to the request's parameter bag.

Note: That endpoints are no enpoints to the repository, means no endpoints on the content path. That shoul be done by the ResourcesRestBunle.

wouterj commented 8 years ago

I don't think we need an extra bundle for this:

Just like @dbu, I don't want to be too negative, but we're a very small team. I don't think we should maintain many different ways of doing the same thing. We already maintain CreatePHP, ResourceRest and Admin, also maintaining yet another way of editing documents which is quite similar to what ResourceRest will look like in the future seems to be not worth it imo.

ElectricMaxxx commented 8 years ago

@WouterJ i wouldn't be that sad, when copying that code into RoutingBundle. I just needed a clean evironment to test and get a feeling. When all want it that way i will push that code into RoutingBundle after the release. An other reason for an own bundle was the issue https://github.com/symfony-cmf/symfony-cmf/issues/180 created by @lsmith77

dbu commented 8 years ago

the controller_by_method enhancer sounds like a good idea to me and could cover most of what you propose with little extra code.

ElectricMaxxx commented 8 years ago

indeed, that's is really a small amount of code. So we could at that to our routing-bundle, right.

But:

Btw 1.0: i do not wanna fight for an extra bundle, just want to point on all use cases. And we used to separate independent stuff in extra bundles in the past. And i would see me as an maintainer of that peace of code.

Btw 2.0: we should do the architectural discussions in the https://github.com/symfony-cmf/symfony-cmf/issues/180. It was my fault to post here and not there. Here we would simply implement the RESTContentController, which would be happy to get some matching routes.

dantleech commented 8 years ago

I am not too deep in this thread, but I thought I would just mention RoutingAuto here.

You will soon be able to generate multiple routes for each Entity/Document. So this would enable you to generate not only the front-end route, but also backend routes if required. Each of which can have a type assigned to it in order to enable routing to the correct controller.