move, rename delete resource of editable repository

[wouterj]

Continues #31 , but then with a branch in the main repo. This way, we can work on it together and get things finished fast.

Depends on

• [WIP] Support for move and remove resource#23
• Test move add remove resource-bundle#31

[ElectricMaxxx]

Sorry i have no rights on all resource packages. Maybe i can get them as on all other CMF packages. Will have time on monday morning to work on. When somebody comes First just do it.

[wouterj]

Sorry i have no rights on all resource packages. Maybe i can get them as on all other CMF packages.

You should have push access now.

[wouterj]

Pushed some commits to use the operation technique I proposed and also fix some testing.

I can't make the testing work though, as I can't find a way to update the DocumentManager. The document is actually moved, but the manager used by the test doesn't sync.

Calling $this->manager->clear() doesn't work, as PHPCR will throw an exception because the previously known node /cms/articles/foo is removed. /cc @dbu @dantleech can you help out here?

[dantleech]

@wouterj could it be that you are starting the PHPCR session in one process (creating the fixtures), moving in another (i.e. the web server process) and asserting using the first again? I have encountered that issue.

Maybe you could clear the document manage / logout of the PHPCR session before launching the browser test?

[wouterj]

Got it working by using Session#refresh(true) and DM#clear(). Not really nice DX, but it works.

This means that move + delete is now implemented. /cc @dantleech @ElectricMaxxx ready for a review

[ElectricMaxxx]

do we really need that operation? Is there no chance to change the path of the resource information or the target path of the request to use that as an information? to have some kind of operation key in the body is not very RESTful.

[wouterj]

Did you read http://williamdurand.fr/2014/02/14/please-do-not-patch-like-an-idiot/ ?

You can use whatever format you want as [description of changes], as far as its semantics is well-defined. That is why using PATCH to send updated values only is not suitable.

RFC 6902 defines a JSON document structure for expressing a sequence of operations to apply to a JSON document, suitable for use with the PATCH method. Here is how it looks like:

[
    { "op": "test", "path": "/a/b/c", "value": "foo" },
    { "op": "remove", "path": "/a/b/c" },
    { "op": "add", "path": "/a/b/c", "value": [ "foo", "bar" ] },
    { "op": "replace", "path": "/a/b/c", "value": 42 },
    { "op": "move", "from": "/a/b/c", "path": "/a/b/d" },
    { "op": "copy", "from": "/a/b/d", "path": "/a/b/e" }
]

[ElectricMaxxx]

OK i understand it now.

[dbu]

could it be that the move happens on phpcr level instead of phpcr-odm document manager? or, as dan suggested, that it happens in separate sessions? if neither is the case, this sounds like a bug. though it must be a complicated case, as the straightforward move test is green: https://github.com/doctrine/phpcr-odm/blob/master/tests/Doctrine/Tests/ODM/PHPCR/Functional/MoveTest.php and also https://github.com/doctrine/phpcr-odm/blob/master/tests/Doctrine/Tests/ODM/PHPCR/Functional/MoveByAssignmentTest.php

[wouterj]

@dbu yeah, what dan suggested is exactly the situation here. However, I would expect Manager#clear() to solve the issue, but that's not the case. While clearing, it calls JackalopeItem#getIdentifier(), which checks the state and errors that the state is incorrect (node was removed). That's a bit strange to me (as I'm clearing it because I know the known state is invalid).

[dbu]

hm, indeed that sounds like a bug in the clear logic. i guess the exception would need to be caught and ignored.

[dbu]

do we need to apply styleci to master and rebase this branch?

anything left to do? as the workaround for the phpcr-odm clear bug is in the tests, i guess we can merge with it. a report on phpcr-odm would be good though, or maybe a test to reproduce the situation.

[wouterj]

If @dantleech and @ElectricMaxxx agree, let's merge.

StyleCI failures are indeed not related to this PR and should be fixed in master instead afaics.

[dantleech]

+1 could we squash into two commits?

[ElectricMaxxx]

👍

[ElectricMaxxx]

Maybe we can merge the styleCI hints first.

[wouterj]

Squashed the commits

[dbu]

s/$param/@param/

[wouterj]

Updated the PR, ready to merge when the build passes.

[dbu]

great! thanks all!