PropertyStorage fix #474
PropertyStorage fix #474
Conversation
| public function __call($method, $args) | ||
| { | ||
| if (is_callable([$this, $method])) { | ||
| return call_user_func_array($this->$method, $args); |
There was a problem hiding this comment.
$this->pathFilter is a public property, so there is no method?
this implementation feels misleading...
There was a problem hiding this comment.
I am not sure either if this is a good solution. $pathFilter is supposed to be a callable (see the section about restricting paths here: http://sabre.io/dav/property-storage/), but currently the $this->pathFilter($path) calls in this class will fail with Call to undefined method Sabre\DAV\PropertyStorage\Plugin::pathFilter() as calling $this->pathFilter() in an object context will try to call a method called pathFilter() which does not exist.
Another option would be to actually add a method pathFilter() and return the property $pathFilter - if it's a callable.
This PR is mostly a bug report, because right now the following calls to $this->pathFilter() in the Property Storage Plugin will all fail:
- https://github.com/DominikTo/sabre-dav/blob/property-storage-fix/lib/DAV/PropertyStorage/Plugin.php#L86
- https://github.com/DominikTo/sabre-dav/blob/property-storage-fix/lib/DAV/PropertyStorage/Plugin.php#L103
- https://github.com/DominikTo/sabre-dav/blob/property-storage-fix/lib/DAV/PropertyStorage/Plugin.php#L119
- https://github.com/DominikTo/sabre-dav/blob/property-storage-fix/lib/DAV/PropertyStorage/Plugin.php#L135
- https://github.com/DominikTo/sabre-dav/blob/property-storage-fix/lib/DAV/PropertyStorage/Plugin.php#L138
/cc @evert
There was a problem hiding this comment.
I think we should change the callsites so PHP don't need to guess if we like to call a property or a method.
just introduce a local var and you are good to go.
Instead of
if ($this->pathFilter && !$this->pathFilter($path)) return;just do
$pathFilter = $this->pathFilter;
if ($pathFilter && !$pathFilter($path)) return;This would make it obvious I think. this could of course be done in a private method, so this "hack" won't be distributed across the whole class..
private function pathFilter($path) {
// make sure we are calling the callable of our public property and not the equal named method
$pathFilter = $this->pathFilter;
return ($pathFilter && !$pathFilter($path));
}WDYT?
There was a problem hiding this comment.
That indeed seems to be a better solution. Also added a test now.
Let's see what @evert thinks. :)
There was a problem hiding this comment.
Great to have a test for this. Looks good to me, thanks.
|
Looks great. |
|
Weird that there were no travis builds ? |
|
Thanks for merging! Travis builds are showing up for me? :) |
Methods and properties are in a different namespace, so currently $this->pathFilter() is an undefined method which causes: