New PR for the cache function #7
Conversation
|
Can you see why travis fails? Composer install fails in PHP 5.6? |
| @@ -57,9 +73,10 @@ class Configuration | |||
| * @access public | |||
| * @param string|null $defaultConfigurationFile Location of the default XML configuration file | |||
| * @param string|null $xsdSchemaFile Location of the XSD file for configuration validation. Optional so that a default schema can also be provided by a subclass. | |||
| * @param boolean|null $useCaching Boolean indicating if caching should be used | |||
There was a problem hiding this comment.
I think $useCaching should only be a boolean.
|
I've commented on some of the code inline. Thanks for changing the performance unit test to a separate script as it wasn't really testing a unit. Please change the filename to lowercase though, as the camelcasing implies it being a class. Maybe name performance test or something. Also I don't understand why you didn't make it do like a 1000 cycles with cached an non-cached. Another thing I don't understand is why you named the file optional.xml for the unit test. |
|
I've restarted the CI test for PHP5.6 and it runs fine. Must have been a glitch with Travis CI. |
…trict boolean checking, updated test to load null
|
I've updated the PR according to your suggestions |
| $this->cache["alwaysArray"][$xpathExpression] = $this->getFromDOMDocument($xpathExpression, $alwaysReturnArray); | ||
| } | ||
| return $this->cache["alwaysArray"][$xpathExpression]; | ||
| } elseif ($this->useCaching) { |
There was a problem hiding this comment.
Please add strict comparison here too.
|
Yay, almost there! I've commented on a few last things inline. We'll leave the array wrapping on-the-fly out of this PR. Could you also run the php-cs-fixer? I'm seeing some violations. ;-) |
| */ | ||
| require '../vendor/autoload.php'; | ||
|
|
||
| foreach ([2, 3] as $count) { |
There was a problem hiding this comment.
This foreach can be removed?
There was a problem hiding this comment.
Well, it can but it'd change that the scripts tests. You could add a 4 and a 5 (or others) to the array as well, just depends on what you want to script to test. I choose to test the performance benefits when calling the same get 2 or 3 times, obviously you'd benefit more from caching as you call a function that can return a cached variable more often.
|
Are you still able to process the remaining feedback to complete this PR? If not, I'll be happy to complete it. |
|
Feel free to complete it. |
Hi,
The rebasing gave me many merge conflicts so I decided to refork and put everything in a new PR. The only open thing I see is your comment about wrapping the array on the fly, see my note in the other PR about this.