Expand references in phpcr:dump

[dantleech]

This PR expands reference in the PHPCR dump to change:

./app/console doctrine:phpcr:dump /cms/chronology --props 
  2011-01-20:
    - references = Array(    [0] => 78b6f31b-f72a-4514-8f9d-b6e4a6786134    [1] => 896a3903-2614-4f8d-
8543-0758022aaa4d    [2] => 43d60...
    - phpcr:classparents = Array()
    - phpcr:class = DTL\TravelBundle\Document\ChronoDate

to

./app/console doctrine:phpcr:dump /cms/chronology --props --expand-references
  2011-01-20:
    - references = 
       - ref: /cms/content/DTLs Blog/aut-molestiae-rerum-eum-eius
       - ref: /cms/media/e6d2cab1959d05eeeec8ebe6bb3f1f3068e89f4e.jpeg
       - ref: /cms/media/49c33d6063f1ad0f3d4a3aa7d09b986c4ccddd02.jpeg
       - ref: /cms/media/34f453a6b65d58345e1ff639b5714da451698178.jpeg
    - phpcr:classparents = Array()
    - phpcr:class = DTL\TravelBundle\Document\ChronoDate

This is really good for debugging, not so sure on the name of the option however. I also wonder if we shouldn't list array properties in a similar way.

[lsmith77]

i guess this also includes #49, i like the feature .. we could even consider enabling it by default.

[dantleech]

On Mon, Apr 15, 2013 at 01:43:12AM -0700, Lukas Kahwe Smith wrote:

i guess this also includes [1]#49, i like the feature .. we could even consider enabling it by default.

The only problem there would be the performance penalty. There is a significant increase in HDD noise from my machine with this option enabled.

— Reply to this email directly or [2]view it on GitHub.

References

Visible links

1. Added support for only removing children https://github.com/phpcr/phpcr-utils/issues/49
2. https://github.com/phpcr/phpcr-utils/pull/50#issuecomment-16373239

[dbu]

i like this. its debug only, so we can live with the performance i guess. i suggest we turn the option into --no-dereference and do your behaviour by default. if you can adapt the purge option name, i can merge #49 and then you can rebase this PR on master and we only see things related to the dump command change. or cherry-pick this commit into a new branch.

[dantleech]

updated. got into a bit of a tangle by accidentally committing a revert, but have reverted the revert. so all good now.

[dbu]

what about the suggestion to invert the default and have a --no-dereference option instead?

[dantleech]

hmm. not sure. it could really be a problem if you have a node that reference like 20000 things, thats 20000 lookups. the performance would be quite non-linear, you would have to avoid dumping certain branches of your tree.

we could maybe list the UUIDs in this format by default, as I don't think that would cost anything extra. then change the option to --show-paths or similar.

[dbu]

ok, good point. lets keep like this then. we already list the reference uuids if props are enabled, right?

btw, how do --expand-references and --props relate to each other? what happens if i only specify --expand-references? i think it should implicitly activate the --props option.

[dantleech]

owe currently just do something like print_r($value, true), and truncate the resulting string. but what do you think to this:

some-references-field:
  - uuid: jru1743nueighqj4378ruwnfh1k68371
  - uuid: jru1743nueighqj4378ruwnfh1k68371
  - uuid: jru1743nueighqj4378ruwnfh1k68371
  - uuid: jru1743nueighqj4378ruwnfh1k68371
  - .. 27 more

and then have an option --list-paths and --list-limit=5 or something.

[dantleech]

oh, and yes, --expand-references only makes sense with --props. Not sure if we should couple the two options though, seems like a premature optimization :) Maybe we could just throw an exception if --props hasn't been specified

[dbu]

the limits things is a general issue about all properties, not limited to references. lets not mix this into this PR.

i would suggest to have --expand-references automatically enable --props, that is more user friendly. i think --expand-references is nicer than --list-paths, so lets keep the name and just handle the --props issue in this PR, then merge.

[lsmith77]

we could also allow to do --props as well as --props=expand-references instead off the additional option.

[dantleech]

Just thinking, gramatically that doesn't work:

dump (with) props
dump (with) props (and) expand references
dump expand references

Don't like --props=expand-references that limits any other options we might add in the future.

I also wonder if --expand-references isn't really --uuid-as-path and if it perhaps couldn't have a broader scope.

Lots of ideas not enough time, and this isn't something thats going to break BC in the future unless somebody uses this for scripting (is there a use case for that?).

Will make the change as you suggest tomorrow morning.

[dbu]

yeah lets just --expand-references automatically make --props active for now. i am not too afraid of changing that later.

[lsmith77]

well --props=expand-references|something|somethingelse can be used if we eventually have more than one thing we want to allow for --props

[dantleech]

Have discussed this a bit with @lsmith on irc, I quite like just adding a --ref-format=[path|uuid] and treating that like a configuration preference, so if --props is not specified, well, we just don't show the properties as normal.

If there are no objections I'll update the PR tomorrow at some point.

[dbu]

ok, makes sense. i did not get to tag releases on friday. if you could do this by tomorrow, it would get into the release ;-)

[dantleech]

ok. updated:

With path:

$ ./app/console doctrine:phpcr:dump --props /cms/chronology --ref-format=path

  2013-04-05:
    - phpcr:class = DTL\TravelBundle\Document\ChronoDate
    - phpcr:classparents = Array()
    - references = 
     - path: /cms/media/e74ca772b78b91c8cb6836ffe9733dcfcdc46d4f.jpeg
     - path: /cms/media/44038ef86981c05f2712e1efbef08919c0491122.jpeg

Or with UUID (note changed format to match above)

$ ./app/console doctrine:phpcr:dump --props /cms/chronology

2013-03-20:
    - phpcr:class = DTL\TravelBundle\Document\ChronoDate
    - phpcr:classparents = Array()
    - references = 
     - uuid: fb4ae65e-3138-4df0-b8f0-ca199f0b0cd3
     - uuid: f8bb5b47-2c17-4f84-899b-fc49e55e926f

[dbu]

cool, the new format make sense to me. there is a perfromance issue and a naming issue i point out above.

[dantleech]

Updated.

[dbu]

apart from the code shortening suggestion with the array cast, looks good to me now.

[dantleech]

Thats a nice trick :) Updated again.

[dbu]

cool, thanks a lot dan. do you know if we have any detailed documentation on this somewhere that now needs update? we should probably tell people to use the help rather than have too much of those details copied into documentation that slowly drift out of sync with the actual code...

[dantleech]

No, nothing existing that is detailed just

./phpcr-odm-documentation/en/reference/tools.rst:49 ./symfony-cmf-docs/bundles/phpcr-odm:750

Both are lists of available commands.

On Mon, Apr 22, 2013 at 02:36:37AM -0700, David Buchmann wrote:

cool, thanks a lot dan. do you know if we have any detailed documentation on this somewhere that now needs update? we should probably tell people to use the help rather than have too much of those details copied into documentation that slowly drift out of sync with the actual code...

— Reply to this email directly or [1]view it on GitHub.

References

Visible links

1. https://github.com/phpcr/phpcr-utils/pull/50#issuecomment-16773620

[dbu]

cool, then we don't need to update doc