Add weak reference path type with bc break #116

Merged
merged 3 commits into from Mar 7, 2012

Conversation

Projects
None yet
2 participants
Member

lsmith77 commented Feb 27, 2012

this PR builds on #113, but breaks BC by renaming the "weak" property to "strategy" and switching from a mice boolean/string to a string property.

Member

dbu commented Mar 7, 2012

i still think it would be better to have an attribute on the PATH property mapping to have it dereference the path, to make the difference to (weak/strong) references more visible and map the phpcr model more closely. but if you are still convinced, i can live with this. before we merge and bc break i have to things i want to discuss:

  1. what if the PATH points to something not existing? of course it should not result in an exception, its like a weak reference. but will the property be null? or the path string (which would be confusing i guess). what happens when i save the node, do i lose the PATH property? if not, how can i delete it if i actually want to?
  2. from #113 but still valid: a PATH property can be set to a relative path (which reduces the problem of moves). i could say my node article (path /my/article) has a property "mainImage" which contains a path like "attachedImages/test.png". if i do getNode() on that property, it is resolved to "/my/article/attachedImages/test.png". move the article to /your/article and everything still works.
    when doing this in the odm, what path would we use? absolute path? that loses quite a bit of the power of the PATH property semantics. one thing we could do is strip the common part of the paths and then store a relative path from there, possible with ../ - but you can create cases where this is the wrong thing to do. should we add some hint for a strategy that creates relative paths?
  3. just an idea: we are somewhat inconsistent. for property mappings, we have multiple=true for multivalue. here we use ReferenceOne and ReferenceMany. those words come from the ORM world, where its a huge difference wheter you have one or several references. in phpcr, its just having multivalue or not, which is a minor detail. what about calling it Reference with the optional multiple=true ? might be not a good idea, just wanted to mention it as now would be a good time to change when we bc break anyways.
Member

lsmith77 commented Mar 7, 2012

Again imho I think you are over thinking this. This feature is an advanced performance optimization.

  1. you can remove a node by simply calling remove() on the document instance. this is the same as with other references. i don't see the difference.

  2. we could also adjust path properties when we do a move, but this would require recursing etc. so i wouldn't do anything there.

  3. i have also thought about that, but imho its cleaner this way to make it clear that there is a lot of "special" logic here. same for @Child vs @Children

@lsmith77 lsmith77 Merge remote-tracking branch 'origin/master' into add_weak_reference_…
…path_type_with_BC_break

Conflicts:
	lib/Doctrine/ODM/PHPCR/ReferenceManyCollection.php
e555520
Member

dbu commented Mar 7, 2012

  1. this is a misunderstanding. i am talking about this situation:
  • repository contains only 1 node : /node1
  • node1 is the document, it has a ReferenceOne property with the path /node1/node3
  • this resolves to $doc->reference == null probably. what happens if i save the document? will i lose "/node1/node3" from my property? or will it be kept? what if i want to remove it?
  1. agreed that adjusting the paths automatically sounds complicated. if you need it use uuid. but what about helping to have relative paths? bad idea?

  2. agreed.

Member

lsmith77 commented Mar 7, 2012

  1. i don't know what will happen in that case .. but i also do not know what will happen on that case with a uuid and a weak reference .. and if you want to remove the "reference" then just assign null?
  2. what do you mean with "helping to have relative paths"? you mean being able to specify that the path should not be stored as an absolute path but instead as a relative path? i guess that would be a nice feature. we could handle that with a new "strategy". aka "relative_path"
Member

dbu commented Mar 7, 2012

  1. ok, then lets open a new issue for that. its the same problem actually with weak references. we should know what happens and think what makes sense. http://www.doctrine-project.org/jira/browse/PHPCR-58

  2. exactly. except that i think a user might need to attach his own strategy to the document manager. we could call this path_relative= and provide "max" that string-compares the target path and the node path and puts as few ../ as necessary. but again this does not block this pull request. http://www.doctrine-project.org/jira/browse/PHPCR-59

so lets merge this. sorry for having been so insistent.

@dbu dbu added a commit that referenced this pull request Mar 7, 2012

@dbu dbu Merge pull request #116 from doctrine/add_weak_reference_path_type_wi…
…th_BC_break

Add weak reference path type with bc break
382ba38

@dbu dbu merged commit 382ba38 into master Mar 7, 2012

Member

lsmith77 commented Mar 7, 2012

ok great .. thx for opening those tickets

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