Allow node names to be passed to getNodeName

[dantleech]

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

[dbu]

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]

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]

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]

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]

@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]

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]

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

can you update the PR?

[dbu]

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

[dantleech]

Updated.

[dbu]

great, thanks!

[dantleech]

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]

this was because of very bad mocking code in TestCase, see jackalope/jackalope#199