added some uses to make code prettier :) #335
Conversation
| @@ -242,7 +251,7 @@ public function getNode($path) | |||
|
|
|||
| if (false !== ($result = $this->caches['nodes']->fetch($cacheKey))) { | |||
| if ('ItemNotFoundException' === $result) { | |||
| throw new ItemNotFoundException(sprintf('Item "%s" not found in workspace "%s"', $path, $this->workspaceName)); | |||
| throw new ItemNotFoundException("Item '$path' not found in workspace '$this->workspaceName'"); | |||
There was a problem hiding this comment.
i prefer sprintf over string inlining. can you please revert this change?
and i thought $this->workspaceName would need to be in {} to work.
There was a problem hiding this comment.
Ok, although I like string inlining because it's shorter and I read article about string inlining for micro optimization: https://blog.blackfire.io/php-7-performance-improvements-encapsed-strings-optimization.html :-D :-D
I will revert this change :-)
| @@ -467,12 +482,12 @@ public function getNodePathForIdentifier($uuid, $workspace = null) | |||
|
|
|||
| $this->assertLoggedIn(); | |||
|
|
|||
| $cacheKey = "nodes by uuid: $uuid, ".$this->workspaceName; | |||
| $cacheKey = "nodes by uuid: $uuid, $this->workspaceName"; | |||
There was a problem hiding this comment.
does this work without {}? can we switch this to sprintf as well?
There was a problem hiding this comment.
I think that works in higher PHP version it's my fault that I have bad version setup in PHPStorm. :(
|
a, very interesting, did not know that. i guess when we switch to php 7
only in a version 2 someday, we should indeed drop sprintf
|
|
Ok, I will revert inlining strings :) |
|
David, I look here: https://3v4l.org/LRIDe according to this test I think that syntax without sprintf works in old unmaintained PHP version too :) Can I let strings inlined in this PR? |
|
fair enough. and all those string operations are when throwing exceptions, which will be slow anyways, and is not the expected case, so i guess we don't care much about php 5 performance of those. |
|
Thank you for your feedback and merge! :) I think that pros of inlined string is better readability (at least for me, I know that it's s subjective) :) |
|
I think that now we must fix PHP 5.6 dependencies because it must work on PHP 5.6 :) And what do you think to mark as skipped PHPCR\Tests\PhpcrUtils\CndParserTest:: testScannerErrorComment to fix tests in PHP7 until we find where is problem? |
|
I think that now we must fix PHP 5.6 dependencies because it must work
on PHP 5.6 :)
yep, that would be cool
And what do you think to mark as skipped
PHPCR\Tests\PhpcrUtils\CndParserTest:: testScannerErrorComment to fix
tests in PHP7 until we find where is problem?
yeah lets do that, then we can at least run the tests with php 7.
created phpcr/phpcr-utils#168 so we keep track
of there being a bug that should be fixed eventually
|
No description provided.