Skip to content

add failing test for #659 - #661

Open
uwej711 wants to merge 3 commits into
doctrine:2.0.xfrom
uwej711:fix_move_children
Open

add failing test for #659#661
uwej711 wants to merge 3 commits into
doctrine:2.0.xfrom
uwej711:fix_move_children

Conversation

@uwej711

@uwej711 uwej711 commented Sep 18, 2015

Copy link
Copy Markdown
Contributor

so there a two conditions to make that test fail

  1. add an preUpdate Listener to the parent class
  2. make sure the parents ChildrenCollection is initialized
    without either one of these conditions the test passes!

@uwej711

uwej711 commented Sep 21, 2015

Copy link
Copy Markdown
Contributor Author

when we restrict preUpdate to fields only, there remains one failing test (see travis). Not sure, if this is the way to go. But I see no other simple solution for #659. I think it would be safer anyway to restrict the possible changes to a documents fields as there a no other changesets computed after the event. But that currently leaves us with no way to reset the order of children (maybe this should be allowed).

@lsmith77 lsmith77 added review and removed wip/poc labels Sep 23, 2015
@uwej711

uwej711 commented Sep 30, 2015

Copy link
Copy Markdown
Contributor Author

I think this needs some discussion ...

@dbu

dbu commented Oct 9, 2015

Copy link
Copy Markdown
Member

events during the flush seem to be a really tricky beast. do you see a way to allow field updates and reorderings, but no renaming? or could we store the renaming operation in a way that the second changeset calculation can check if something was renamed and not get confused?

could we look at object identity rather than paths? uuid would only be something for 2.0 as we would change everything to have a uuid.

@uwej711

uwej711 commented Oct 9, 2015

Copy link
Copy Markdown
Contributor Author

I can try to look into the reordering but I fear this will not be an easy thing to fix.

@dbu

dbu commented Oct 10, 2015 via email

Copy link
Copy Markdown
Member

@uwej711

uwej711 commented Oct 11, 2015

Copy link
Copy Markdown
Contributor Author

That's already done here, with the one test about reverting the reordering of the children in preUpdate failing, i.e. that would be a bc break of some kind.

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.

3 participants