Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Child annotation and file handling #4

Merged
merged 12 commits into from May 9, 2011

Conversation

Projects
None yet
3 participants
Contributor

uwej711 commented Mar 20, 2011

Here is a first version of a child annotation that allows for "embedded" documents. The name of the child is determined by the annotation. A simple example is the child jcr:content required by the nt:file node type.

I also added the custom mixin phpcr:managed and Document classes for File and Folder. For usage refer to ChildTest and FileTest in the functional tests directory.

Note: to be able to use the ODM with this patch it is necessary to register the node type phpcr:managed as described in the readme.

Any feedback is welcome.

Kind regards
Uwe

Uwe Jäger added some commits Mar 19, 2011

Uwe Jäger Added Child annotation and File document
Now you can include children in your documents by annotating them with
Child. The ODM will automatically create a child node in the repository
with the given name (if a document is assigned ...).
A useful example is the File document that has a child of class
Resource.
09c5591
Uwe Jäger Rebased current master and clean up
From master id ggeneration was introduced. Made it work with current
changes.
3a97568
Uwe Jäger Exception when a child is replaced by another object
Throw an exception when someone tries to replace a child object with
another object.
9846982

@lsmith77 lsmith77 and 1 other commented on an outdated diff Mar 21, 2011

lib/Doctrine/ODM/PHPCR/Id/IdGenerator.php
@@ -3,6 +3,7 @@
namespace Doctrine\ODM\PHPCR\Id;
use Doctrine\ODM\PHPCR\DocumentManager;
+use Doctrine\ODM\PHPCR\Mapping\ClassMetadata;
@lsmith77

lsmith77 Mar 21, 2011

Member

i fixed this issue in #5

@uwej711

uwej711 Mar 21, 2011

Contributor

The tests did not run :-)

@lsmith77

lsmith77 Mar 21, 2011

Member

Ok, did a few fixes in the tests, now I am down to:
Tests: 69, Assertions: 101, Errors: 19, Incomplete: 2, Skipped: 2.

BTW by default the tests use a "tests" workspace that you need to manually create:
http://www.eppelheimer.com/clients/day/jackrabbit/site/apache/faq.html#create-workspace

@lsmith77 lsmith77 commented on the diff Mar 21, 2011

lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadataInfo.php
@@ -179,6 +179,12 @@ class ClassMetadataInfo implements ClassMetadata
public $associationsMappings = array();
@lsmith77

lsmith77 Mar 21, 2011

Member

FYI: In general i want to see the ClassMetadataInfo implementations of MongoDB, CouchDB and PHPCR ODM come together a bit more. Right now I prefer the way MongoDB does reference/embed the most.

@uwej711

uwej711 Mar 21, 2011

Contributor

I didn't look at those but at JCR ... therefore it's named Child now ...
For the reference stuff I think we should also go the JCR way, i.e. have properties fo type Reference or WeakReference on the nodes, multivalued if it is a to-many association.

@lsmith77

lsmith77 Mar 21, 2011

Member

imho in jackalope we should follow JCR/Jackrabbit naming, but in PHPCR ODM we should follow the naming of the other ODM's as much as possible.

@uwej711

uwej711 Mar 21, 2011

Contributor

But what do we do if the concepts behind are different? A child in JCR is just a full-feature repository node as every other - it is visible from outside. Ther e is no notion of containment or anything else ...
Anyway I will look at the other ODMs to.

@lsmith77

lsmith77 Mar 21, 2011

Member

IMHO we should focus PHPCR ODM on the things that are the same or similar. This is also why I am advocating with the renaming of path to id. The main point from my POV is to make it possible to share knowledge and code with the other ODM's. For most PHPCR specific stuff imho developers can fallback to Jackalope. However we can also of course add some connivence features to PHOCR ODM that are PHPCR specific.

So I guess the gist is that we should always carefully review if there are similar concepts on the other ODM's and if we can sensibly adopt the naming form there. Of course this is just my POV.

@uwej711

uwej711 Mar 21, 2011

Contributor

Three last comments here (this should be part of my feedback on the list):

  • If the concepts are the same or similar it should be named the same way and work the same way
  • If it's all the same you loose the good reasons for using phpcr ...
  • The fallback to PHPCR is there but it basically breaks your controllers or services - somewhere you need to place the code. If not in the ODM it will move up ...

@lsmith77 lsmith77 commented on an outdated diff Mar 21, 2011

lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -20,6 +20,7 @@
namespace Doctrine\ODM\PHPCR;
use Doctrine\ODM\PHPCR\Mapping\ClassMetadata;
+use Doctrine\ODM\PHPCR\Mapping\ClassMetadataInfo;
@lsmith77

lsmith77 Mar 21, 2011

Member

looking at MongoDB and CouchDB ODM it looks like they only use ClassMetadataInfo for caching, in the rest of the code they just reference ClassMetadata.

Member

lsmith77 commented Mar 21, 2011

i will try to run the tests tonight .. i need to add tests for the id generator stuff too :-/

@lsmith77 lsmith77 commented on the diff Mar 21, 2011

lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -141,8 +142,8 @@ class UnitOfWork
{
// TODO create a doctrine: namespace and register node types with doctrine:name
- if ($node->hasProperty('_doctrine_alias')) {
@lsmith77

lsmith77 Mar 21, 2011

Member

i do not have a strong opinion about this change. actually i think its good. but its a significant change that we should consciously do and get right the first time around.

@uwej711

uwej711 Mar 21, 2011

Contributor

if this is not done via a proper mixin you cannot use builtin jcr note types. So I think there is no way around doing this ...

@dbu

dbu Mar 21, 2011

Member

i really like the idea. its even better than the namespaced doctrine attribute property we originally had in mind.
it IS a fundamental change, but i am rather confident its a good one, so we should do it as early as possible :-)

is this needed? the call to phpcr Node::remove() does a recursive remove itself.
or do we have to give the child entity a chance to trigger something on application level?

Owner

uwej711 replied Mar 21, 2011

When the child node to be removed has mapped children that are modified flush will try to create or save the nodes which will lead to an exception from Jackalope/Jackrabbit. This code just removes all the documents, it does not schedule the descendants for removal in the content repository. That is taken care of by the Node::remove(), as you say.

dbu replied Mar 22, 2011

ok, good. maybe you could add this as a doc comment for future reference?

please add a short doc comment what difference Remove1 and Remove2 have

why is this not allowed? maybe i missed something on the mailinglist - but it would be good to have a comment right here for other people asking that same question. and some mention of the fact in the doc.

Owner

uwej711 replied Mar 21, 2011

I gave my view on this on the thread about renaming path to id. I think "moving" nodes should be a separate operation. This will also keep the UnitOfWork a bit simpler. To support that fully we need to remove the old child and possibly move the new child. Maybe it's easier to implement than I first thought.

dbu replied Mar 22, 2011

ah, right. and i agreed with you :-D
i just did not really see that this would be a move. maybe you could make the exception text more explicit: "You can not move or copy children by assignment as it would be ambigous. Please use the PHPCR\Session::move() resp PHPCR\Session::copy operations for this."

really a detail: indention is not consistent for the above line

have a doc comment for the setFileContent function? i suppose filename must be an absolute path, right?

would be nice to also have a method to set content with a string directly, so if i generate something in memory i do not have to first write it to a temporary file

Owner

uwej711 replied Mar 21, 2011

This was created mainly as a first use case for the child annotation and I don't consider that finished. I would like to integrate it fully with forms to finish it.

dbu replied Mar 22, 2011

i think it makes sense to have both a setFileContent($content) method and a setFileContentFromFilesystem($absolute_filename), and then another one for the form component if needed, once that component settled.

Member

dbu commented Mar 21, 2011

hi uwe,
thanks a lot for the work you did. i am no doctrine expert, but this looks pretty good to me. i commented on some phpcr related issues. i think its an excellent idea to provide default entities for the built-in file and folder types.

open points to me are

  • is the annotation going to be called child or embed? as we pre-load, i think embed sounds more appropriate. and consistent with the other odm's. even though its not exactly the same as embed in mongodb...
  • we should have some doc comments on the entities pointing to the relevant jcr doc, and doc in other places where appropriate
  • what happens if there are multiple children with the same name? this is allowed by jcr and would result in the names being indexed with [0]... important for now is that the annotation can allow for this case later.
  • lazy loading - but this optimization, we can do that later

why did you define the properties as protected? shouldn't they be public?

Owner

uwej711 replied Mar 21, 2011

I think these should be basic classes that are subclassed by the application/bundle etc. I don't consider File, Folder and Resource completly finished for now. Therefore I started of with protected properties.

dbu replied Mar 22, 2011

ok. although i think it can make sense to use them right away :-)
but either way, please explain in the doc comment what the intention is.

Contributor

uwej711 commented Mar 21, 2011

hi david,

regarding the same name siblings: I think this is more the case for a Children annotation where there is an array of documents as children. If I have two children with the same name I can not know which belongs to which property when recreating the document.

About the naming ... someone has to decide ...

Uwe Jäger Changed some minor issues as commented on the pull request
Added some comments and minor formatting stuff
a4b46b5
Member

dbu commented Mar 22, 2011

about the name: i switch to mailing list for that discussion to hopefully get more input.

same name siblings: for later - there are bugs in jackalope with that anyway. one use case i see: i want to atttach an arbitrary number of pictures or download files to a node. but then, it is probably more clean to do that with a nt:folder and then the nt:files having individual names and get all children of nt:folder.

about the discussion about state of the code: you created a pull request, so i assumed you want your changes merged into the phpcr-odm codebase :-) and to merge it, we should have some comments and stuff, even if its going to change or be improved later on;:-)

Contributor

uwej711 commented Mar 22, 2011

Right - I walk through your comments and try to add the things you suggested.

@uwej711 uwej711 Cleaned up the code according to dbu's recommendation
Added comments and made File, Folder and Resource directly usable.
On test is failing due to a bug in UnitOfWork fixed in master.
5730595
Contributor

uwej711 commented Mar 22, 2011

OK, I added some comments and changed the File and Folder classes to be usable directly. So there seems to be only the naming stuff left ...

Member

dbu commented Apr 9, 2011

hi uwe,
we are getting there :-) with the naming, things should be clear, right?
to merge this pull, the piece missing is a symfony command to wrap the register node command in phpcr-odm (just merged your pull request there). and maybe some sort of check if the node type has been registered. or just capture the exception about the missing type and provide a dev friendly exception explaining he has first to run the command to register the mixin type.
cheers,david

Contributor

uwej711 commented Apr 11, 2011

name is child ... :-)

catching the exception is a bit difficult, since there is no nice exception thrown by Jackalope or Jackrabbit on Node::addMixin (was the case last time I checked - will look at that again). According to the JCR-Spec a NoSuchNodeTypeException should be thrown.

Contributor

uwej711 commented Apr 11, 2011

@dbu: Regarding the exception: the jackrabbit java client checks the node types existence when Node:addMixin is called. The jackrabbit server does no check on Session:save. The spec leaves this to the implementation so to be on the save side we should implement the check in Jackalope Node:addMixin.

Member

dbu commented Apr 11, 2011

you are right. as a first step, i will try to make jackalope at least throw the correct exception on save.

uwej711 added some commits Apr 13, 2011

@uwej711 uwej711 Merge remote-tracking branch 'doctrine/master' into child_annotation 9e75f54
@uwej711 uwej711 Removed Id from fieldMapping
Since the Id is used internally as the nodes path it is not necessary to
store it with the node - it is actually impossible to do it with a
predefined node type that does not allow arbitrary properties as
nt:file. There for we need a different mapping for Id (as it was before
with Path).
89dcc11
Contributor

uwej711 commented Apr 13, 2011

I merged doctrine/master. The renaming of path to id introduced a problem since the id was also stored as a property on the node. This is fixed with 89dcc11.

Member

dbu commented Apr 14, 2011

oh, good catch. and it totally does not make sense to store the id with the node if its the nodes path.

@lsmith77 lsmith77 commented on an outdated diff May 9, 2011

lib/Doctrine/ODM/PHPCR/Document/File.php
+ *
+ * @param string $content the content for the file
+ */
+ public function setFileContent($content)
+ {
+ $this->getContent();
+ $this->content->setData($content);
+ }
+
+ /*
+ * Ensure content object is created
+ */
+ private function getContent()
+ {
+ if ($this->content === null)
+ {
@lsmith77

lsmith77 May 9, 2011

Member

this { should be on the previous line

@lsmith77 lsmith77 commented on an outdated diff May 9, 2011

lib/Doctrine/ODM/PHPCR/UnitOfWork.php
}
- }
+ }
@lsmith77

lsmith77 May 9, 2011

Member

no need to add trailing whitespace :)

@lsmith77 lsmith77 commented on an outdated diff May 9, 2011

lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -618,8 +665,22 @@ class UnitOfWork
$data['doctrine_metadata']['associations'][$fieldName] = $ids;
}
}
+ // child is set to null ... remove the node ...
+ } else if (isset($class->childMappings[$fieldName])) {
@lsmith77

lsmith77 May 9, 2011

Member

PEAR/Symfony2 CS defines that it should be "elseif", but we might need to clean this up in lots of places

@dbu dbu added a commit that referenced this pull request May 9, 2011

@dbu dbu Merge pull request #4 from uwej711/child_annotation
Child annotation and file handling. 
The document meta information is now stored in phpcr:alias property and no longer in the _doctrine_alias. You need to update Jackrabbit. 
See https://github.com/doctrine/phpcr-odm/wiki/Custom-node-type-phpcr:managed

Also updates Jackalope and PHPCR. Changes there:

* we ditched the binary interface (after long discussion) and binary is now working properly. meaning that whereever you get a binary value, it will be a *stream*. if you want to access that, you can stream_get_contents(). or maybe do something more efficient like fpassthru()

* for simplification, Property::getNativeValue is now Property::getValue (with the same semantics)
0fdab3c

@dbu dbu merged commit 0fdab3c into doctrine:master May 9, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment