Skip to content
This repository has been archived by the owner on Sep 16, 2021. It is now read-only.

REST write support #93

Open
lsmith77 opened this issue Nov 29, 2013 · 33 comments
Open

REST write support #93

lsmith77 opened this issue Nov 29, 2013 · 33 comments

Comments

@lsmith77
Copy link
Member

with #91 and schmittjoh/serializer#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
Copy link
Member

dbu commented Feb 14, 2014

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

@lsmith77
Copy link
Member Author

we have read support now .. but no write support

@lsmith77
Copy link
Member Author

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

@dbu
Copy link
Member

dbu commented Oct 28, 2014

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
Copy link
Member Author

@ElectricMaxxx
Copy link
Member

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

@lsmith77
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

dbu commented Dec 19, 2014

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
Copy link
Member

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
Copy link
Member

dbu commented Feb 3, 2016 via email

@lsmith77
Copy link
Member Author

lsmith77 commented Feb 3, 2016

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

@ElectricMaxxx
Copy link
Member

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
Copy link
Member

dbu commented Feb 3, 2016 via email

@ElectricMaxxx
Copy link
Member

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

@ElectricMaxxx
Copy link
Member

@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
Copy link
Member

wouterj commented Feb 3, 2016

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
Copy link
Member

@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:

  • routing, here i prefere the solution mentioned in create a RestBundle that creates routes to any content on the fly symfony-cmf#180. Not sure if we need an other bundle or just add it into the routing, but we do need some dynamic route handling to catch all requests next to GET and map them really equal to our routing
  • RDFa support for frontend editing, yes we still need that, but that can be provided by the ResourcesRestBundle
  • ContentBundle as an example implementation for the REST Controller, because i think with the routing mentioned in my first point, a user will be able to map its own ContentRestControllers or just extend ours
  • Javascript application like create which uses the RDFa and the ContentRest API to do the frontend editing

@dbu
Copy link
Member

dbu commented Feb 8, 2016

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
Copy link
Member

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:

  • create routes with method constraints
  • enhancers matching the method and add informtion to route an action
  • one enhancer for all (at least CRUD)
  • or enhancers for eauch method
  • create routes for collection endpoints without any content -> can fetch POST, PUT (move) and GET (listAction)
  • PUT/GET/DELETE can go on the given routes
  • No controller inside RestBundle
  • example implementation in the ContentBundle with a RestController equal to our ContentController
  • sonata admin extension for easy method adding
  • sonata admin extension for collection handling (like reference many fields)

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
Copy link
Member

dbu commented Feb 12, 2016

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:

  • use FOSRest to create documents, find a way so that the client can send an explicit route rather than use RoutingAuto
  • a 404 handler that offers an edit interface to create a new page, posting to the correct REST end points to create content for that URL

@ElectricMaxxx
Copy link
Member

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
Copy link
Member

wouterj commented Feb 12, 2016

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
Copy link
Member

@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
Copy link
Member

wouterj commented Feb 13, 2016

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

  • I think a controller_by_method enhancer makes sense for more than just REST, why not include it in Routing + RoutingBundle?
  • All other actions can be managed by the ResourceRestBundle in the future. We then only need to implement some kind of id-to-resource mapper to map a generic id (in this case a route path) to a resource path (in this case the path of the content bundle).

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
Copy link
Member

@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 symfony-cmf/symfony-cmf#180 created by @lsmith77

@dbu
Copy link
Member

dbu commented Feb 15, 2016

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
Copy link
Member

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

But:

  • What about the possibility to add routes with that methods (admin
    extension)?
  • What about Collection endpoints?

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
symfony-cmf/symfony-cmf#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
Copy link
Member

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants