Add, remove and move #17

ElectricMaxxx · 2016-05-20T13:24:40Z

This PR does the implemenetation of the the base methods of the EditableRepository in both repositories. We will hopefully get the move method when puli/repository#26 is ready, but i will start with it before the merge and use the interface defined by @wouterj

ToDo:

add()
remove()
move()
clear()

Note: i branched from the state #15 will rebase on master when its merged, so the other commits should disappear.

dantleech · 2016-05-20T14:01:46Z

Looks good! will have a proper look later on.

dantleech · 2016-05-20T14:30:43Z

Repository/AbstractPhpcrRepository.php

+
+        $resolvedPath = $this->resolvePath($query);
+
+        $this->removeResource($resolvedPath, $deleted);


Can we change this to:

return $this->removeResource($resolvedPath);

and have removeResource simply return the number of items deleted?

ElectricMaxxx · 2016-05-21T05:59:56Z

hey @dantleech

i did the changes you whant me to do, but i think the thing with the path isn't correct. The injected path can not be the destination path. otherwise adding of a collection of resources will completly wrong. Lets do an example:

$path = /some/path
When adding a node/document with name = path the way we do is correct, we just need to assert the name against the rest of the path. So $parentPath = /some and $localName = path
What would be when adding two resources, one with name = pathA and one with name = pathB, than the localName doesn't match to injected path as the final destination anymore.
I try to imagine creating a file there i would say lets create a file with name bla.tx in directory /home/max so the target directory is our parent path. Same when adding two or more files into one directory.

dantleech · 2016-05-21T08:39:30Z

Repository/PhpcrOdmRepository.php

+
+            $document = $resource->getPayload();
+            $document->setName($resource->getName());
+            if ($document instanceof HierarchyInterface) {


Documents do not require a "name" method, they also do not need to implement the HierearchyInterface in order to have a parent, nor would they necessarily have either a "parent" or a "name" mapping as they could be using an "ID/path" ID strategy (where you would set the path directly).

In the Syluus resource bundle I force the user to use the name and parent mappings, but that could be more flexible.

I am not even sure it makes sense to be able to create documents here at all - you cannot gaurantee that a document doesn't have required fields which cannot be set using this API.

/cc @dbu @wouterj

I think creation and validation of fields/properties should be done outside. i.e. in a controller of rest-resources-bundle - the point where in the input will be created.

Currently creation and edition would be/is done either by SonataAdmin or SyliusResourceBundle - which supports both HTML and REST endpoints for editing and creating new objects.

I see the Resource system as providing links to these endpoints which are capable of dealing with the resource's payload.

I still think we should open complete CRUD endpoints on Phpcr and PhpcrOdm here (over resource-rest-bundle) on the complete node/documents to enable all frontend applications like admins, to work on the path based structure. The content-bundle would give a REST endpoint on based on the content's route (and the dynamic routing) to have CRUD on content only. a use case would be the frontend editing. That would be my big picture in 4 lines :-)

Regarding the above code.

We don't even need to set the name or parent. We are being passed the payload - i.e. the document. It should already be ready-to-persist without further modification or?

dantleech · 2016-05-21T08:42:19Z

On reflection -- what is the use case for "add" ?

With both the ODM and PHPCR the class/node-types may require fields which cannot be set by simply creating a Document/Node.

dantleech · 2016-05-28T08:27:40Z

Tests/Unit/Repository/PhpcrRepositoryTest.php

+    }
+
+    /**
+     * @expectedException \InvalidArgumentException


dantleech · 2016-05-28T08:32:20Z

I am still not sure of why we need add for PHPCR nodes / ODM documents, can somebody give me a use case? /cc @wouterj @webmozart

dantleech · 2016-05-28T08:41:55Z

Repository/AbstractPhpcrRepository.php

+
+        $resolvedPath = $this->resolvePath($query);
+
+        return $this->removeResource($resolvedPath, $deleted);


Passing deleted does not make any sense.

ElectricMaxxx · 2016-05-28T12:49:55Z

Will work on the rest of the hints tonight.

dantleech · 2016-05-31T07:32:48Z

Tests/Fixtures/FalsyResource.php

+/**
+ * @author Maximilian Berghoff <[email protected]>
+ */
+class FalsyResource implements PuliResource


Why not use GenericResource here?

dantleech · 2016-05-31T07:35:22Z

@ElectricMaxxx would you mind if i fork this PR and merge only the move and remove parts? As I currently don't know how add is going to be useful or if we are doing that correctly.

After move and remove are merged this PR will just be about add.

ElectricMaxxx · 2016-05-31T07:49:09Z

👍 can you do same for the bundle?

dantleech · 2016-05-31T07:53:03Z

Will do

dantleech · 2016-05-31T07:59:15Z

Repository/AbstractPhpcrRepository.php

+    /**
+     * {@inheritdoc}
+     */
+    public function remove($query, $language = 'glob')


@ElectricMaxxx I think this is for removing multiple resources? But we are only removing one?

dantleech · 2016-06-02T09:57:50Z

Forked and replaced by: #23

dantleech reviewed May 20, 2016
View reviewed changes

dantleech reviewed May 21, 2016
View reviewed changes

ElectricMaxxx added 14 commits May 21, 2016 22:30

fix tests

2b4fb5a

revert test change on time

f6ba659

use right test case, add version test

f149889

replace an other test case

e4f7b87

remove unused use statements

243fc79

get getVersion() implementation and tests

95dc52d

remove unused use statements

d8c42ee

implement add for PhpcrOdmRepository

1082fc6

implement add for PhpcrRepository

d2a4a96

implement remove on both repositories

d6836e6

fix some stype

1fd94bb

make styleCi happy

e133c30

make styleCi happy v01i

f664291

changes by hints in PR

6fb12ec

dantleech reviewed May 28, 2016
View reviewed changes

Tests/Unit/Repository/PhpcrRepositoryTest.php

}

/**

* @expectedException \InvalidArgumentException

Copy link

Member

dantleech May 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

message

dantleech reviewed May 28, 2016
View reviewed changes

current state of changes for better tests

9bb5f05

ElectricMaxxx added 5 commits May 28, 2016 20:57

revert moved/deleted return, test on exception messages

e9616ec

introduce CmfEditableRepository for move()

1846a25

clean up properties

6ca6676

make styleCI happy

6b5b441

move child property defintions

1bbdda8

dantleech reviewed May 31, 2016
View reviewed changes

dantleech mentioned this pull request May 31, 2016

Re-order interface / method #20

Closed

dantleech reviewed May 31, 2016
View reviewed changes

dantleech mentioned this pull request Jun 2, 2016

[WIP] Support for move and remove #23

Merged

dantleech closed this Jun 2, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add, remove and move #17

Add, remove and move #17

ElectricMaxxx commented May 20, 2016 •

edited

Loading

dantleech commented May 20, 2016

dantleech May 20, 2016

ElectricMaxxx commented May 21, 2016

dantleech May 21, 2016

ElectricMaxxx May 21, 2016

dantleech May 21, 2016

ElectricMaxxx May 21, 2016

dantleech May 23, 2016

ElectricMaxxx May 23, 2016 via email

dantleech commented May 21, 2016

dantleech May 28, 2016

dantleech commented May 28, 2016

dantleech May 28, 2016

ElectricMaxxx commented May 28, 2016

dantleech May 31, 2016

dantleech commented May 31, 2016

ElectricMaxxx commented May 31, 2016 via email

dantleech commented May 31, 2016

dantleech May 31, 2016

dantleech commented Jun 2, 2016


		$resolvedPath = $this->resolvePath($query);

		$this->removeResource($resolvedPath, $deleted);


		$resolvedPath = $this->resolvePath($query);

		return $this->removeResource($resolvedPath, $deleted);

Add, remove and move #17

Add, remove and move #17

Conversation

ElectricMaxxx commented May 20, 2016 • edited Loading

dantleech commented May 20, 2016

Choose a reason for hiding this comment

ElectricMaxxx commented May 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElectricMaxxx May 23, 2016 via email

Choose a reason for hiding this comment

dantleech commented May 21, 2016

Choose a reason for hiding this comment

dantleech commented May 28, 2016

Choose a reason for hiding this comment

ElectricMaxxx commented May 28, 2016

Choose a reason for hiding this comment

dantleech commented May 31, 2016

ElectricMaxxx commented May 31, 2016 via email

dantleech commented May 31, 2016

Choose a reason for hiding this comment

dantleech commented Jun 2, 2016

ElectricMaxxx commented May 20, 2016 •

edited

Loading