Expand references in phpcr:dump #50

dantleech · 2013-04-15T08:40:41Z

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 · 2013-04-15T08:43:11Z

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

dantleech · 2013-04-15T08:43:53Z

src/PHPCR/Util/Console/Helper/TreeDumper/ConsoleDumperPropertyVisitor.php

I generally prefer to pass in an $options array and do an array_merge with defaults afterwards to avoid argument hell, not sure what other people prefer. But, maybe an argument for "expandReferences" makes more sense here anyhow.

i am fine with this approach for scalar options.

dantleech · 2013-04-15T08:57:52Z

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

Added support for only removing children
Added support for only removing children #49

Expand references in phpcr:dump #50 (comment)

dbu · 2013-04-15T09:21:55Z

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 · 2013-04-16T12:18:37Z

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

dbu · 2013-04-16T12:23:11Z

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

- If specified, the full path to each referrer is listed beneath each reference node instead of the normal "print_r" value. - Really good for debugging...

This reverts commit 7c8be1c.

This reverts commit 9820847.

dantleech · 2013-04-16T15:31:48Z

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 · 2013-04-16T15:39:47Z

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 · 2013-04-16T16:00:34Z

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 · 2013-04-16T16:05:40Z

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 · 2013-04-16T16:27:47Z

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 · 2013-04-16T17:16:46Z

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

dantleech · 2013-04-16T18:04:57Z

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 · 2013-04-19T13:04:57Z

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

lsmith77 · 2013-04-19T13:06:19Z

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

dantleech · 2013-04-20T19:16:31Z

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 · 2013-04-21T07:48:06Z

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 · 2013-04-21T09:36:08Z

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

- Also list UUIDs in same way as we list paths

dbu · 2013-04-21T12:05:14Z

src/PHPCR/Util/Console/Helper/TreeDumper/ConsoleDumperPropertyVisitor.php

the problem with this is that it will fetch the node. if you want to show uuid, this is pretty expensive - the main reason to show uuid is exactly the performance impact if we have many references.

could you change this to only do getValue if we need the paths and otherwise get the uuids as string? i am not 100% sure if the fact that the keys of that array are uuid is just implementation detail.

btw, its references here, not referrers. referrers would be nodes that reference this node.

dbu · 2013-04-21T12:06:06Z

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

dantleech · 2013-04-22T08:45:04Z

Updated.

dbu · 2013-04-22T08:49:36Z

src/PHPCR/Util/Console/Helper/TreeDumper/ConsoleDumperPropertyVisitor.php

i saw @lsmith77 using (array) to simplify this. found http://www.php.net/manual/en/language.types.array.php#language.types.array.casting that explains this feature. this could further shorten your code here.

dbu · 2013-04-22T08:50:28Z

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

dantleech · 2013-04-22T08:57:13Z

Thats a nice trick :) Updated again.

Expand references in phpcr:dump

dbu · 2013-04-22T09:36:36Z

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 · 2013-04-22T10:14:33Z

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

Expand references in phpcr:dump #50 (comment)

dbu · 2013-04-22T10:42:16Z

cool, then we don't need to update doc

dantleech reviewed Apr 15, 2013
View reviewed changes

dantleech added 3 commits April 16, 2013 14:09

Added "expand references" option to dump command

c73a2a9

- If specified, the full path to each referrer is listed beneath each reference node instead of the normal "print_r" value. - Really good for debugging...

Revert "Added support for only removing children"

9820847

This reverts commit 7c8be1c.

Revert "Revert "Added support for only removing children""

9874686

This reverts commit 9820847.

Refactored for --ref-format

454eb9b

- Also list UUIDs in same way as we list paths

dbu reviewed Apr 21, 2013
View reviewed changes

Performance fix

4a9a2ca

dbu reviewed Apr 22, 2013
View reviewed changes

Cast values to array

9cc0852

dbu added a commit that referenced this pull request Apr 22, 2013

Merge pull request #50 from dantleech/expand_references

602c504

Expand references in phpcr:dump

dbu merged commit 602c504 into phpcr:master Apr 22, 2013

Expand references in phpcr:dump #50

Expand references in phpcr:dump #50

Uh oh!

Conversation

dantleech commented Apr 15, 2013

Uh oh!

lsmith77 commented Apr 15, 2013

Uh oh!

dantleech Apr 15, 2013

Choose a reason for hiding this comment

Uh oh!

lsmith77 Apr 15, 2013

Choose a reason for hiding this comment

Uh oh!

dantleech commented Apr 15, 2013

Uh oh!

dbu commented Apr 15, 2013

Uh oh!

dantleech commented Apr 16, 2013

Uh oh!

dbu commented Apr 16, 2013

Uh oh!

dantleech commented Apr 16, 2013

Uh oh!

dbu commented Apr 16, 2013

Uh oh!

dantleech commented Apr 16, 2013

Uh oh!

dantleech commented Apr 16, 2013

Uh oh!

dbu commented Apr 16, 2013

Uh oh!

lsmith77 commented Apr 16, 2013

Uh oh!

dantleech commented Apr 16, 2013

Uh oh!

dbu commented Apr 19, 2013

Uh oh!

lsmith77 commented Apr 19, 2013

Uh oh!

dantleech commented Apr 20, 2013

Uh oh!

dbu commented Apr 21, 2013

Uh oh!

dantleech commented Apr 21, 2013

Uh oh!

dbu Apr 21, 2013

Choose a reason for hiding this comment

Uh oh!

dbu commented Apr 21, 2013

Uh oh!

dantleech commented Apr 22, 2013

Uh oh!

dbu Apr 22, 2013

Choose a reason for hiding this comment

Uh oh!

dbu commented Apr 22, 2013

Uh oh!

dantleech commented Apr 22, 2013

Uh oh!

dbu commented Apr 22, 2013

Uh oh!

dantleech commented Apr 22, 2013

Uh oh!

dbu commented Apr 22, 2013

Uh oh!

Uh oh!