-
Notifications
You must be signed in to change notification settings - Fork 15
[WIP] move, rename delete resource of editable repository #31
Changes from all commits
e09bb32
7231043
e80602c
e6de622
01f30df
fcd3f0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ php: | |
- 5.6 | ||
- 7.0 | ||
- hhvm | ||
|
||
sudo: false | ||
|
||
cache: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,10 +11,16 @@ | |
|
||
namespace Symfony\Cmf\Bundle\ResourceRestBundle\Controller; | ||
|
||
use PHPCR\PathNotFoundException; | ||
use PHPCR\Util\PathHelper; | ||
use Puli\Repository\Api\EditableRepository; | ||
use Puli\Repository\Api\ResourceRepository; | ||
use Symfony\Cmf\Component\Resource\RepositoryRegistryInterface; | ||
use Symfony\Component\HttpFoundation\Request; | ||
use Symfony\Component\HttpFoundation\Response; | ||
use JMS\Serializer\SerializerInterface; | ||
use JMS\Serializer\SerializationContext; | ||
use Symfony\Component\Routing\Exception\RouteNotFoundException; | ||
|
||
class ResourceController | ||
{ | ||
|
@@ -29,7 +35,8 @@ class ResourceController | |
private $serializer; | ||
|
||
/** | ||
* @param RepositoryInterface | ||
* @param SerializerInterface $serializer | ||
* @param RepositoryRegistryInterface $registry | ||
*/ | ||
public function __construct( | ||
SerializerInterface $serializer, | ||
|
@@ -39,11 +46,103 @@ public function __construct( | |
$this->registry = $registry; | ||
} | ||
|
||
public function resourceAction($repositoryName, $path) | ||
public function getResourceAction($repositoryName, $path) | ||
{ | ||
$repository = $this->registry->get($repositoryName); | ||
$resource = $repository->get('/'.$path); | ||
|
||
return $this->createResponse($resource); | ||
} | ||
|
||
/** | ||
* There are two different changes that can currently be done on a cmf resource: | ||
* | ||
* - move | ||
* - rename | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can move only change the parents but not the name itself? does rename allow to only specify the new name but not the path? not sure if that would make worth it to have two different operations. you still need to know the full identifier of the resource. or is this a puli thing or otherwise required to be two different operations? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the way I understand it, you move the resources to a new parent path and you cannot rename them with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok. feels unintuitive from a filesystem point of view. but if this is
not our choice but puli or something else having this logic, it works as
well.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At least my move PR on Puli does support renaming in the move action (as @dbu said, both actions are just about changing paths): https://github.com/puli/repository/pull/26/files#diff-14ae094154456c4c7c9fa3ddb909c559R388 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that makes sense. it works the same as the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dantleech that would be awesome if it fieels equal. The Comment maybe can be seen as a hint, what to document afterwards, cause we need to explain it. |
||
* | ||
* changing payload properties isn't supported yet. | ||
* | ||
* @param string $repositoryName | ||
* @param string $path | ||
* @param Request $request | ||
* | ||
* @return Response | ||
*/ | ||
public function patchResourceAction($repositoryName, $path, Request $request) | ||
{ | ||
$repository = $this->registry->get($repositoryName); | ||
$this->failOnNotEditable($repository, $repositoryName); | ||
|
||
$resourceName = $request->get('node_name'); | ||
|
||
$targetPath = null; | ||
if ($path !== $path) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this doesn't make sense anymore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest, I don't understand much of the code in this controller. Isn't this what you want: function patchResourceAction($repositoryName, $path, Request $request)
{
$repository = $this->registry->get($repositoryName);
$this->failOnNotEditable($repository, $repositoryName);
$targetPath = $request->get('node_name'); // "id" seems like a better name for this
try {
$repository->move($path, $targetPath);
} catch (\Exception $e) {
return $this->createBadRequestResponse($e->getMessage()); // why not rely on Symfony's great error handler here?
}
$resource = $repository->get($targetPath);
return $this->createResponse($resource); // not sure whether we should really return the resource here, I think we should sent a 200 response without body
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, I think we should take http://williamdurand.fr/2014/02/14/please-do-not-patch-like-an-idiot/ into account. This way, we can already open this controller for more options (like editing), but at the moment we can add a guard to only handle There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. Was a stupid rename.
|
||
$targetPath = $path; | ||
} elseif ($resourceName !== PathHelper::getLocalNodeName(PathHelper::absolutizePath($path))) { | ||
$targetPath = PathHelper::absolutizePath($repositoryName, PathHelper::getParentPath($path)); | ||
} | ||
|
||
if (null == $targetPath) { | ||
return $this->badRequestRespons('Move and rename is supported only.'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo: |
||
} | ||
|
||
try { | ||
$repository->move($path, $targetPath); | ||
} catch (\InvalidArgumentException $e) { | ||
return $this->badRequestResponse($e->getMessage()); | ||
} | ||
|
||
$resource = $repository->get($targetPath); | ||
|
||
return $this->createResponse($resource); | ||
} | ||
|
||
/** | ||
* Delete a resource of a repository. | ||
* | ||
* @param string $repositoryName | ||
* @param string $path | ||
* | ||
* @return Response | ||
*/ | ||
public function deleteResourceAction($repositoryName, $path) | ||
{ | ||
$repository = $this->registry->get($repositoryName); | ||
$this->failOnNotEditable($repository, $repositoryName); | ||
|
||
try { | ||
$repository->remove($path); | ||
} catch (\InvalidArgumentException $e) { | ||
return $this->badRequestResponse($e->getMessage()); | ||
} | ||
|
||
return $this->createResponse('', Response::HTTP_NO_CONTENT); | ||
} | ||
|
||
private function failOnNotEditable(ResourceRepository $repository, $repositoryName) | ||
{ | ||
if (!$repository instanceof EditableRepository) { | ||
throw new RouteNotFoundException(sprintf('Repository %s is not editable.', $repositoryName)); | ||
} | ||
} | ||
|
||
/** | ||
* @param string $message | ||
* | ||
* @return Response | ||
*/ | ||
private function badRequestResponse($message) | ||
{ | ||
return $this->createResponse(['message' => $message], Response::HTTP_BAD_REQUEST); | ||
} | ||
|
||
/** | ||
* @param object|array $resource | ||
* @param int $httpStatusCode | ||
* @return Response | ||
*/ | ||
private function createResponse($resource, $httpStatusCode = Response::HTTP_OK) | ||
{ | ||
$context = SerializationContext::create(); | ||
$context->enableMaxDepthChecks(); | ||
$context->setSerializeNull(true); | ||
|
@@ -53,7 +152,7 @@ public function resourceAction($repositoryName, $path) | |
$context | ||
); | ||
|
||
$response = new Response($json); | ||
$response = new Response($json, $httpStatusCode); | ||
$response->headers->set('Content-Type', 'application/json'); | ||
|
||
return $response; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,8 +11,7 @@ | |
|
||
namespace Symfony\Cmf\Bundle\ResourceRestBundle\Enhancer; | ||
|
||
use JMS\Serializer\Context; | ||
use Puli\Repository\Api\Resource\PuliResource; | ||
use Puli\Repository\Api\Resource\Resource; | ||
|
||
/** | ||
* Enhancer classes enhance the REST response for resources. | ||
|
@@ -28,8 +27,8 @@ interface EnhancerInterface | |
* | ||
* $context->addData('foobar', 'Some value'); | ||
* | ||
* @param Context Serialization context | ||
* @param resource The resource being serialized | ||
* @param [] $data Context Serialization context | ||
* @param Resource $resource The resource being serialized | ||
*/ | ||
public function enhance(array $data, PuliResource $resource); | ||
public function enhance(array $data, Resource $resource); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wouterj i did the change here, but i think it is the other way around as you said it. Or do we we have some version proplems in our chain from resource -> resource-bundle -> resource-restbundle ->sandbox? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now I am completely confused. Localy i still got There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had to explicitly include Puli
On Fri, Jun 03, 2016 at 06:38:48AM -0700, Maximilian Berghoff wrote:
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
# Symfony CMF Resource REST API Bundle | ||
|
||
[![Build Status](https://secure.travis-ci.org/symfony-cmf/ResourceRestBundle.png?branch=master)](http://travis-ci.org/symfony-cmf/ResourceRestBundle) | ||
[![Build Status](https://travis-ci.org/symfony-cmf/resource-rest-bundle.svg?branch=master)](https://travis-ci.org/symfony-cmf/resource-rest-bundle) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the build icon change all other bundles still have. |
||
[![StyleCI](https://styleci.io/repos/29090266/shield)](https://styleci.io/repos/29090266) | ||
[![Latest Stable Version](https://poser.pugx.org/symfony-cmf/resource-rest-bundle/version.png)](https://packagist.org/packages/symfony-cmf/resource-rest-bundle) | ||
[![Total Downloads](https://poser.pugx.org/symfony-cmf/resource-rest-bundle/d/total.png)](https://packagist.org/packages/symfony-cmf/resource-rest-bundle) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,26 @@ | ||
_cmf_resource: | ||
_delete_cmf_resource: | ||
path: /api/{repositoryName}/{path} | ||
methods: ['delete'] | ||
requirements: | ||
path: .* | ||
defaults: | ||
_controller: cmf_resource_rest.controller.resource:resourceAction | ||
_controller: cmf_resource_rest.controller.resource:deleteResourceAction | ||
_format: json | ||
|
||
_patch_cmf_resource: | ||
path: /api/{repositoryName}/{path} | ||
methods: ['patch'] | ||
requirements: | ||
path: .* | ||
defaults: | ||
_controller: cmf_resource_rest.controller.resource:patchResourceAction | ||
_format: json | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this wouldn't change anything (unless I'm wrong) |
||
|
||
_get_cmf_resource: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer to put the vendor in front: |
||
path: /api/{repositoryName}/{path} | ||
methods: ['get'] | ||
requirements: | ||
path: .* | ||
defaults: | ||
_controller: cmf_resource_rest.controller.resource:getResourceAction | ||
_format: json |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,3 +90,30 @@ Feature: PHPCR-ODM resource repository | |
} | ||
} | ||
""" | ||
@doNow | ||
Scenario: Rename a PHPCR-ODM resource | ||
Given there exists a "Article" document at "/cmf/articles/foo": | ||
| title | Article 1 | | ||
| body | This is my article | | ||
Then I set header "Content-Type" with value "application/json" | ||
When I send a PATCH request to "/api/phpcrodm_repo/foo" with body: | ||
""" | ||
{"node_name": "foo-bar"} | ||
""" | ||
Then the response code should be 200 | ||
When I send a GET request to "/api/phpcrodm_repo/foo-bar" | ||
Then the response code should be 200 | ||
And the response should contain json: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Imo, this is not how BDD should be done. We're way to close to the implementation at this point. I would rather suggest:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 and using the DM to assert the Change under the "then statement"?
|
||
""" | ||
{ | ||
"repository_alias": "phpcrodm_repo", | ||
"repository_type": "doctrine_phpcr_odm", | ||
"payload_alias": "article", | ||
"payload_type": "Symfony\\Cmf\\Bundle\\ResourceRestBundle\\Tests\\Resources\\TestBundle\\Document\\Article", | ||
"path": "\/foo-bar", | ||
"node_name": "foo-bar", | ||
"label": "foo-bar", | ||
"repository_path": "\/foo-bar", | ||
"children": [] | ||
} | ||
""" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
*/ | ||
|
||
$container->setParameter('cmf_testing.bundle_fqn', 'Symfony\Cmf\Bundle\ResourceRestBundle'); | ||
$container->setParameter('kernel.environment', 'test'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should not be needed if the Kernel is instantiated correctly (with the test env). |
||
$loader->import(CMF_TEST_CONFIG_DIR.'/dist/parameters.yml'); | ||
$loader->import(CMF_TEST_CONFIG_DIR.'/dist/framework.php'); | ||
$loader->import(CMF_TEST_CONFIG_DIR.'/dist/monolog.yml'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the styleci complains because the first line of a comment is the short description.
we should say something like "Change an existing resource." and then add a new line.