Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add mapping to index reference collection by node name #702

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

koemeet
Copy link
Contributor

@koemeet koemeet commented Apr 18, 2016

Fixes #701

  • Implement annotations and YAML driver
  • Restrict it only to only reference-many collections?

… associative indexes based on the reference node name.
@koemeet koemeet closed this Apr 18, 2016
@lsmith77 lsmith77 removed the wip/poc label Apr 18, 2016
@koemeet koemeet reopened this Apr 18, 2016
@koemeet koemeet changed the title Add index-by-nodename XML mapping, so you can make a reference have… [WIP] Add mapping to index reference collection by node name Apr 18, 2016
@koemeet
Copy link
Contributor Author

koemeet commented Apr 18, 2016

@dbu So, for a reference-many collection it should be okay to maybe do this behaviour by default? Then we could even get rid of the whole index-by-nodename mapping. Never worked with the referrers relations etc. to much, but I think the reference-many is always unique, right?

@dbu
Copy link
Member

dbu commented Apr 19, 2016

for the ReferenceManyCollection, i am not sure if we can do the associative by name at all, because of the risk of name collisions. maybe we should use the full path instead of the name for the reference many collection.
with references, you can reference arbitrary documents. if you reference /a/b/c and /b/b/c then both local names will be "c".

children are handled with the ChildrenCollection. for children collection, i agree that having them associative would be a good idea.

i think we should have the configuration option for now, but tell that in version 2 we will only have associative children and remove the option again. there is a risk of unexpected behaviour when the keys change.

$referencedDocs[] = $proxy;
if ($isAssociative) {
$index = $referencedNode->getName();
$referencedDocs[$index] = $proxy;
Copy link
Member

Choose a reason for hiding this comment

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

this might overwrite other referenced documents with the same node name. we should use $referenceNode->getPath() i think.

@dbu
Copy link
Member

dbu commented Jun 20, 2016

i just started with a 2.0 branch. maybe this should be done against 2.0 so we need not worry about unexpected BC troubles because of slightly different behaviour. wdyt @steffenbrem ?

@dbu
Copy link
Member

dbu commented Jan 17, 2017

@steffenbrem do you want to wrap this up for 2.0?

i think we need to index by path instead of by nodename, to avoid problems with name collisions.

@koemeet
Copy link
Contributor Author

koemeet commented Jan 17, 2017

@dbu Nice! I will take a look at it and implement it to index by path, seems indeed a better approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants