search

phpcr / phpcr-utils

A set of helper classes and command line commands you want to use with phpcr, but that are not part of the API itself
phpcr.github.io
Other
72 stars 30 forks source link

Allow node names to be passed to getNodeName #95

Closed dantleech closed 10 years ago

dantleech commented 10 years ago

This was required in my shell where the path arguments are sometimes relative node names,

dbu commented 10 years ago

Hm, the phpdoc ja quite clear that you must pass valid absolute paths to this method. Can you Post a Link where you call this? If its user input, you might rather absolutize and normalize first. Imagine i type .. to get up one level.

dantleech commented 10 years ago

Infact its used in phpcr-utils: https://github.com/phpcr/phpcr-utils/blob/master/src/PHPCR/Util/Console/Command/NodeTouchCommand.php#L108

If the spec says the path should be absolute I should fix that in my code -- but maybe we should throw an exception if there are no path delimiters in the given "path", otherwise

$nodeName = PathHelper::nodeName('foobar');
echo $nodeName;

yields oobar which isn't right.

dbu commented 10 years ago

okay, oobar is certainly not good :-) we could validate that the path is absolute and throw an exception otherwise. we could call self::assertValidAbsolutePath with the default parameters so that it throws an exception.

regardless of this, the touch command should validate that the path is absolute and throw an error if its not absolute. there is no context available there, and i think there is little value in creating nodes unter the root node if you do not specify the leading slash.

dbu commented 10 years ago

what should we do here? validate the parameter to getNodeName? i am a bit concerned about the overhead we generate for this. maybe we should just insist that this must be an absolute path, or do a simpler check for a leading slash?

and add validation in the touch command parameter?

dbu commented 10 years ago

@dantleech would you agree with my proposal to add validation in the touch command and just specify that the client code must be sure to pass in a valid absolute path to getNodeName? constantly validating any parameters could get rather expensive.

dantleech commented 10 years ago

Not sure, I think if we allow this behavior it is going to make people scratch their heads in the future. But looking at the code again, we could do this without any "overhead":

$strpos = strpos($path, '/');
if (false === $strpos) { throw Exception(); }
return substr($path, $strpos + 1);
dbu commented 10 years ago

okay, that makes sense. lets use self::error(sprintf('Path '%s' is not an absolute path', $path));

can you update the PR?

dbu commented 10 years ago

if you can update this way, we can merge and tag 1.1.0-RC1 hopefully.

dantleech commented 10 years ago

Updated.

dbu commented 10 years ago

great, thanks!

dantleech commented 10 years ago

hmm, this causes problems in jackalope-doctrine-dbal:

1) Jackalope\Observation\EventFilterIdentifiersTest::testFilter

PHPCR\RepositoryException: Path "" must be an absolute path

/home/travis/build/jackalope/jackalope-doctrine-dbal/vendor/phpcr/phpcr-utils/src/PHPCR/Util/PathHelper.php:269

/home/travis/build/jackalope/jackalope-doctrine-dbal/vendor/phpcr/phpcr-utils/src/PHPCR/Util/PathHelper.php:237

/home/travis/build/jackalope/jackalope-doctrine-dbal/vendor/jackalope/jackalope/src/Jackalope/Item.php:180

/home/travis/build/jackalope/jackalope-doctrine-dbal/vendor/jackalope/jackalope/src/Jackalope/Node.php:1410

/home/travis/build/jackalope/jackalope-doctrine-dbal/vendor/jackalope/jackalope/src/Jackalope/Item.php:155

/home/travis/build/jackalope/jackalope-doctrine-dbal/vendor/jackalope/jackalope/src/Jackalope/Node.php:107

/home/travis/build/jackalope/jackalope-doctrine-dbal/vendor/jackalope/jackalope/tests/Jackalope/TestCase.php:116

/home/travis/build/jackalope/jackalope-doctrine-dbal/vendor/jackalope/jackalope/tests/Jackalope/Observation/EventFilterIdentifiersTest.php:79

/home/travis/build/jackalope/jackalope-doctrine-dbal/vendor/jackalope/jackalope/tests/Jackalope/Observation/EventFilterIdentifiersTest.php:62

/home/travis/build/jackalope/jackalope-doctrine-dbal/vendor/jackalope/jackalope/tests/Jackalope/Observation/EventFilterIdentifiersTest.php:12
dbu commented 10 years ago

this was because of very bad mocking code in TestCase, see https://github.com/jackalope/jackalope/pull/199