Add a REST module, starting with DELETE

Some quick comments as I go through:

Don't bother with the rest_resource_manager() function. Just make code access the container directly.

Routes should technically begin with a / now. The path matcher for various reasons is currently accounting for both approaches, but Symfony assumes a leading /. We should do that.

As discussed on G+, the access() method on Resources should probably get removed and handled at the route level instead.

"Router" as a class name is going to confuse people. :-) Also, that class should be made ContainerAware, and then you can call $this->container->get('resource_manager')->getInstance() instead.

For EntityResource, do we want methods of CRUD, or do we want to map those to HTTP methods (GET, PUT, DELETE, etc.)? That could actually simplify the "Router" class quite a bit.

The list of HTTP methods is fixed. It doesn't change per site. Rather than have that mapping array hard coded somewhere, just call the PHP methods strtolower($http_method). So HTTP POST => public function post(), etc.

I don't see what the extra abstraction layer here buys us. PUT and PATCH are not the same thing and not interchangeable, so if you swap "update" from one to the other you're changing semantics and changing the API, which is bad.

If those are controllers, then they should not be defined explicitly by an interface.

/node/{node}

That would map to:

public function get(Node $node) {}
public function put(Node $node) {}
public function delete(Node $node) {}

And you just don't implement the rest. Whereas this:

/foo/{node}/{user}/{view}

Maps to:

public function get(Node $node, User $user, View $view) {}
public function post(User $user, View $view,Node $node) {} // Order is irrelevant, remember

And that's all you implement.

Make more sense? (I suppose this means you maybe need a method_exists() check, either at runtime or when generating routes based on the information that call directly to the method.

Meant to refile...

This might be moot given #11.1, but just in case it's not:

Loading the node just to delete it feels a bit silly

You can have a controller signature of delete($node) without type hinting it with Node, and then just the raw id will be passed. Or you can make the signature delete($nid), but then the route needs to be /node/{nid} instead of /node/{node}.

hook_route_info() relied on the loaded plugin manager which was not available immediately.

That should actually be fixed as of #1708692-51: Bundle services aren't available in the request that enables the module. Might therefore be possible to revert #13.

+   * Plugins can override this method to provide additional custom request
+   * methods.

That's so cool. :-)

+  protected function requestMethods() {
...
  'DELETE'

If request methods can be defined within plugin derivatives, why is it necessary to hard-code any request methods? Could you move these method declarations directly into the plugin derivatives where they are used?

Perhaps this would help to dynamically build the list of available methods when responding to an OPTIONS request of a particular route.

+   *   The list of allowed HTTP request method strings.

If you keep some methods hard-coded, then I think "common HTTP request methods", as expressed in
HTTP/1.1: Method Definitions is a more maleable description.

+          'format' => 'application/ld+json',

Is configurging accepted mime types going to be added in this issue? There's also: #1505080: [META] Content Negotiation for Accept Headers, and a list of mime types is available in ./core/includes/file.mimetypes.inc, so perhaps you could leverage that.

I wonder if there might be another approach that would make it possible to select request methods and mime types in a route configuration UI.

Looking great to me. Some nits ...

The admin page at admin/config/services/rest is a bit too plain for my taste. I personally would like some grouping of all entity types and descriptions for the enity group. I think some Help for this admin page would be good. You could reuse text from the Issue Summary here. I might also like to see which http methods are supported for each resource. And example CURL commands for invoking them :). And pictures of muscular German developers making DELETE requests :)

I think the watchdog service should be renamed to dblog service and should ship inside of dblog folder. I don't know if there is a more appropriate place for the EntityResource plugin to live.

I'd love for the dblog to be able to return a collection like the most recent n entries of a given type. See `drush --filter=watchdog` for all the operations we could support. We might as well start figuring out the API for this. I agree with doing this in a followup as you suggest.

Typo: defintion in ResourceManager class

Typo: tst in DeleteTest doxygen

Typo: enttiy in entityCreate() in ResourceBase.php

// Clear the static cache, otherwise we won't see the update. in testDelete(). This comment seems not to match the code and can be removed.

I think httpRequest method could go right into WebTestBase class

Can an $entity->delete() fail? We don't handle that in EntityResource::delete

@moshe

I'd love for the dblog to be able to return a collection like the most recent n entries of a given type.

Is there an already resolved decision about things like paging, queries, and collection handling?

At this point, my view is that collections and paging are a problem for Views plugins, because Views is how we build collections.

The details of what Views should output in that case in terms of the exact output, I don't know.

#23: Maybe as a follow-up to this issue, #1819760: Add a REST export display plugin and serializer integration., and #23298: Entity bundles should declare a plural form of their label; in #1792828: [META] Provide entity integration with Views, support for entity type machine name as a contextual filter could be added, and then a default route for '/{entity-type-name-plural}' could supply the collection.

Pluralizing things sometimes is not good. Your top-level resource list gets cluttered, and you can't easily extend "node/" to "node/32" to go from collection/factory to entity.

Let me reiterate: Collection GET routes are off topic for this thread. :-) We're just dealing with a framework for single-item CRUD. Views is how we create collection listings, and therefore a given site builder could put whatever collection listing he wants wherever he wants.

If we want core to ship with some default views or template views for listing services, that's a February/March question, not a November question.

I think plugins are used very nicely here. Really like this approach.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.phpundefined
@@ -0,0 +1,90 @@
+        $route = new Route("/$prefix/{id}", array(
+          '_controller' => 'Drupal\rest\RequestHandler::handle',
+          'plugin' => $this->plugin_id,
+        ), array(
+          '_method' => $method,
+          'format' => 'application/ld+json',
+        ));

For the POST request (to create an entity) we should post to "/$prefix" as we don't have the id.

Maybe we should remove 'format' property for now. As now we aim for delete call to be working it should be ok to do it with any other format.

We can also improve testing (to provide verbose message about headers and body of request plus headers and body of response). This can be done in follow-up issue.

format is needed even for delete, in case there are errors. in any case, i think consistency is valuable here. I don't think the tests need to be more verbose, as an error message with a line number will identify exactly what the problem is. we have been removing assert messages in tests recently for this reason.

i looked at the code and I think we can commit it just to keep things moving. we have to work on CREATE/READ/UPDATE soon. We can move the dblog service in a follow-up.

Note: On routes, non-prefixed defaults and requirements have special meaning. Defaults get used for placeholders, and requirements are treated as a regex for a matching placeholder. That's why there's _method and _controller. plugin and format need to be prefixed as well, I believe.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/Derivative/EntityDerivative.phpundefined
@@ -0,0 +1,50 @@
+    return $this->derivatives[$derivative_id];

I might be wrong, but shouldn't there be an isset() check ?

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.phpundefined
@@ -0,0 +1,90 @@
+          'title' => t('Access @method on resource %label', array('@method' => $method, '%label' => $definition['label'])),

Nitpick, but grammatically shouldn't this be "Access @method on %label resource" ? ("Access POST on node resource")

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/Type/ResourceManager.phpundefined
@@ -0,0 +1,48 @@
+class ResourceManager extends PluginManagerBase {

I think the current usage in core is to name the classes FooBarPluginManager.
(same way we do FooBarCacheBackend - the name of the generic 'thing' that is implemented is PluginManager, CacheBackend, not Manager, Backend)

+++ b/core/modules/rest/rest.installundefined
@@ -0,0 +1,15 @@
+/**
+ * Implements hook_uninstall().
+ *
+ * @todo is this needed when using the config system?
+ */
+function rest_uninstall() {
+  config('rest')->delete();

I don't think this is needed, or at least there's an open issue for it, and other modules in core currently don't bother with this, so I guess that could be left out of this patch.

+++ b/core/modules/rest/rest.moduleundefined
@@ -0,0 +1,101 @@
+        $plugin = $manager->createInstance($key);

The usual Plugin API flow is to call getInstance(), while createInstance() is internal.

At least, ResourceManager ::getInstance() currently contains some logic wrapping createInstance(), but is never actually called in the patch.

+++ b/core/modules/rest/rest.moduleundefined
@@ -0,0 +1,101 @@
+        $plugin = $manager->createInstance($key);

Ditto

22 days to next Drupal core point release.

klausi: Request attributes that have magic special meaning to the system (e.g., for routing purposes) should have a _ prefix. Request attributes that are just values intended for the controller to take as a parameter should not have a prefix.

I'm not 100% sure which category plugin would fall into here, but _format definitely needs a prefix since that will be used for routing.

Here is updated patch adding comments about how we can use _format after #1793520: Add access control mechanism for new router system committed.

All recent feedback is in. RTBC.

This looks great, and has buy-in from all the right people. Committed to 8.x. Yay! :-)

Moving the change notice requirement from #1935538-22: Switch REST to default to HAL to here, since I don't think we have any change notice at all yet for rest.module. The bulk of the info should be added as pages within http://drupal.org/developing/api, with a change notice just announcing the new module and pointing there.

Thanks klausi.

:)

Fallout from this: #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources.