From 620a5fa33aed1df985e5bfc365a55579e0c0596c Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Wed, 22 Oct 2025 18:55:19 +0200 Subject: [PATCH 1/4] perf: avoid array_merge in loops Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- lib/DAV/CorePlugin.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/DAV/CorePlugin.php b/lib/DAV/CorePlugin.php index ac9292801b..afe150e607 100644 --- a/lib/DAV/CorePlugin.php +++ b/lib/DAV/CorePlugin.php @@ -334,10 +334,11 @@ public function httpPropFind(RequestInterface $request, ResponseInterface $respo // Normally this header is only needed for OPTIONS responses, however.. // iCal seems to also depend on these being set for PROPFIND. Since // this is not harmful, we'll add it. - $features = ['1', '3', 'extended-mkcol']; + $features = [['1', '3', 'extended-mkcol']]; foreach ($this->server->getPlugins() as $plugin) { - $features = array_merge($features, $plugin->getFeatures()); + $features[] = $plugin->getFeatures(); } + $features = array_merge(...$features); $response->setHeader('DAV', implode(', ', $features)); $prefer = $this->server->getHTTPPrefer(); From 59429221f0e8798483a34cb36f06ee4cd50a2184 Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Wed, 22 Oct 2025 20:21:43 +0200 Subject: [PATCH 2/4] refactor: split httpPropFind for readability Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- lib/DAV/CorePlugin.php | 72 +++++++++++++++++++++++++++++------------- 1 file changed, 50 insertions(+), 22 deletions(-) diff --git a/lib/DAV/CorePlugin.php b/lib/DAV/CorePlugin.php index afe150e607..c3f0ca0c3d 100644 --- a/lib/DAV/CorePlugin.php +++ b/lib/DAV/CorePlugin.php @@ -303,28 +303,12 @@ public function httpDelete(RequestInterface $request, ResponseInterface $respons */ public function httpPropFind(RequestInterface $request, ResponseInterface $response) { - $path = $request->getPath(); - - $requestBody = $request->getBodyAsString(); - if (strlen($requestBody)) { - try { - $propFindXml = $this->server->xml->expect('{DAV:}propfind', $requestBody); - } catch (ParseException $e) { - throw new BadRequest($e->getMessage(), 0, $e); - } - } else { - $propFindXml = new Xml\Request\PropFind(); - $propFindXml->allProp = true; - $propFindXml->properties = []; - } - - $depth = $this->server->getHTTPDepth(1); - // The only two options for the depth of a propfind is 0 or 1 - as long as depth infinity is not enabled - if (!$this->server->enablePropfindDepthInfinity && 0 != $depth) { - $depth = 1; - } - - $newProperties = $this->server->getPropertiesIteratorForPath($path, $propFindXml->properties, $depth); + $propFindXml = $this->parseRequestedProperties($request); + $newProperties = $this->server->getPropertiesIteratorForPath( + $request->getPath(), + $propFindXml->properties, + $this->getDepth() + ); // This is a multi-status response $response->setStatus(207); @@ -905,4 +889,48 @@ public function getPluginInfo() 'link' => null, ]; } + + /** + * Parses the PROPFIND request body and returns a PropFind XML object. + * + * If the request body contains XML, it is parsed using the server's XML service + * and must represent a {DAV:}propfind element. If the body is empty, a default + * PropFind(XML) object is created that requests all properties. + * + * @param RequestInterface $request + * @return array|Xml\Request\PropFind|string + * @throws BadRequest + */ + public function parseRequestedProperties(RequestInterface $request) + { + $requestBody = $request->getBodyAsString(); + if (strlen($requestBody)) { + try { + $propFindXml = + $this->server->xml->expect('{DAV:}propfind', $requestBody); + } catch (ParseException $e) { + throw new BadRequest($e->getMessage(), 0, $e); + } + } else { + $propFindXml = new Xml\Request\PropFind(); + $propFindXml->allProp = true; + $propFindXml->properties = []; + } + return $propFindXml; + } + + /** + * Returns the appropriate value for depth from the HTTP request. + * + * @return int + */ + protected function getDepth(): int + { + $depth = $this->server->getHTTPDepth(1); + // The only two options for the depth of a propfind is 0 or 1 - as long as depth infinity is not enabled + if (!$this->server->enablePropfindDepthInfinity && 0 != $depth) { + $depth = 1; + } + return $depth; + } } From 47c2c7e2c76d3116a2782e0c34833a1b6a100404 Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Wed, 22 Oct 2025 20:30:21 +0200 Subject: [PATCH 3/4] feat: add beforePropertyResolution event This allows interested plugins to filter and slice the list of items for which properties are discovered, thus allowing the implementation of pagination logic that actually discovers properties for a small set of nodes at a time. Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- lib/DAV/CorePlugin.php | 20 +++-- lib/DAV/Server.php | 66 +++++++++++--- tests/Sabre/DAV/CorePluginTest.php | 135 ++++++++++++++++++++++++++++- 3 files changed, 195 insertions(+), 26 deletions(-) diff --git a/lib/DAV/CorePlugin.php b/lib/DAV/CorePlugin.php index c3f0ca0c3d..da422072b0 100644 --- a/lib/DAV/CorePlugin.php +++ b/lib/DAV/CorePlugin.php @@ -304,11 +304,13 @@ public function httpDelete(RequestInterface $request, ResponseInterface $respons public function httpPropFind(RequestInterface $request, ResponseInterface $response) { $propFindXml = $this->parseRequestedProperties($request); - $newProperties = $this->server->getPropertiesIteratorForPath( + $propFindRequests = $this->server->generatePropFindsForPath( $request->getPath(), $propFindXml->properties, $this->getDepth() ); + $this->server->emit('beforePropertyResolution', [&$propFindRequests]); + $fileProperties = $this->server->getNodePropertiesGenerator($propFindRequests); // This is a multi-status response $response->setStatus(207); @@ -328,7 +330,7 @@ public function httpPropFind(RequestInterface $request, ResponseInterface $respo $prefer = $this->server->getHTTPPrefer(); $minimal = 'minimal' === $prefer['return']; - $data = $this->server->generateMultiStatus($newProperties, $minimal); + $data = $this->server->generateMultiStatus($fileProperties, $minimal); $response->setBody($data); // Sending back false will interrupt the event chain and tell the server @@ -891,14 +893,14 @@ public function getPluginInfo() } /** - * Parses the PROPFIND request body and returns a PropFind XML object. + * Parses the PROPFIND request body and returns a PropFind XML object. * - * If the request body contains XML, it is parsed using the server's XML service - * and must represent a {DAV:}propfind element. If the body is empty, a default - * PropFind(XML) object is created that requests all properties. + * If the request body contains XML, it is parsed using the server's XML service + * and must represent a {DAV:}propfind element. If the body is empty, a default + * PropFind(XML) object is created that requests all properties. * - * @param RequestInterface $request * @return array|Xml\Request\PropFind|string + * * @throws BadRequest */ public function parseRequestedProperties(RequestInterface $request) @@ -916,13 +918,12 @@ public function parseRequestedProperties(RequestInterface $request) $propFindXml->allProp = true; $propFindXml->properties = []; } + return $propFindXml; } /** * Returns the appropriate value for depth from the HTTP request. - * - * @return int */ protected function getDepth(): int { @@ -931,6 +932,7 @@ protected function getDepth(): int if (!$this->server->enablePropfindDepthInfinity && 0 != $depth) { $depth = 1; } + return $depth; } } diff --git a/lib/DAV/Server.php b/lib/DAV/Server.php index 5d37dbff9f..8b10ca613e 100644 --- a/lib/DAV/Server.php +++ b/lib/DAV/Server.php @@ -4,10 +4,13 @@ namespace Sabre\DAV; +use Generator; +use Iterator; use Psr\Log\LoggerAwareInterface; use Psr\Log\LoggerAwareTrait; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; +use Sabre\DAV\Exception\NotFound; use Sabre\Event\EmitterInterface; use Sabre\Event\WildcardEmitterTrait; use Sabre\HTTP; @@ -423,7 +426,7 @@ public function getPlugin($name) /** * Returns all plugins. * - * @return array + * @return ServerPlugin[] */ public function getPlugins() { @@ -942,21 +945,20 @@ public function getPropertiesForPath($path, $propertyNames = [], $depth = 0) } /** - * Returns a list of properties for a given path. + * Returns an iterable of tuples, each containing an initialized PropFind object + * and its corresponding node, for the given $path. The node tree is traversed + * starting from the node at $path, according to the specified $depth. * - * The path that should be supplied should have the baseUrl stripped out - * The list of properties should be supplied in Clark notation. If the list is empty - * 'allprops' is assumed. + * Each PropFind represents a resource discovered during traversal but contains + * no property values. The objects are only initialized with the appropriate + * constructor parameters and are meant to be populated later by the property + * retrieval logic. * - * If a depth of 1 is requested child elements will also be returned. - * - * @param string $path - * @param array $propertyNames - * @param int $depth + * @return iterable Iterable of [PropFind, INode] tuples * - * @return \Iterator + * @throws NotFound when a node for $path cannot be found */ - public function getPropertiesIteratorForPath($path, $propertyNames = [], $depth = 0) + public function generatePropFindsForPath($path, $propertyNames = [], $depth = 0): iterable { // The only two options for the depth of a propfind is 0 or 1 - as long as depth infinity is not enabled if (!$this->enablePropfindDepthInfinity && 0 != $depth) { @@ -979,8 +981,22 @@ public function getPropertiesIteratorForPath($path, $propertyNames = [], $depth $propFindRequests = $this->generatePathNodes(clone $propFind, current($propFindRequests)); } - foreach ($propFindRequests as $propFindRequest) { - list($propFind, $node) = $propFindRequest; + return $propFindRequests; + } + + /** + * Yields arrays representing multistatus results with all the discovered + * properties for the provided $propFindRequests. + * + * @see PropFind::getResultForMultiStatus() + */ + public function getNodePropertiesGenerator(iterable $propFindRequests): Generator + { + /** + * @var PropFind $propFind + * @var INode $node + */ + foreach ($propFindRequests as [$propFind, $node]) { $r = $this->getPropertiesByNode($propFind, $node); if ($r) { $result = $propFind->getResultForMultiStatus(); @@ -999,6 +1015,28 @@ public function getPropertiesIteratorForPath($path, $propertyNames = [], $depth } } + /** + * Returns a list of properties for a given path. + * + * The path that should be supplied should have the baseUrl stripped out + * The list of properties should be supplied in Clark notation. If the list is empty + * 'allprops' is assumed. + * + * If a depth of 1 is requested child elements will also be returned. + * + * @param string $path + * @param array $propertyNames + * @param int $depth + * + * @return Iterator + */ + public function getPropertiesIteratorForPath($path, $propertyNames = [], $depth = 0) + { + $propFinds = $this->generatePropFindsForPath($path, $propertyNames, $depth); + + return $this->getNodePropertiesGenerator($propFinds); + } + /** * Returns a list of properties for a list of paths. * diff --git a/tests/Sabre/DAV/CorePluginTest.php b/tests/Sabre/DAV/CorePluginTest.php index 152a50ff55..07cb281f68 100644 --- a/tests/Sabre/DAV/CorePluginTest.php +++ b/tests/Sabre/DAV/CorePluginTest.php @@ -4,11 +4,140 @@ namespace Sabre\DAV; -class CorePluginTest extends \PHPUnit\Framework\TestCase +use Generator; +use PHPUnit\Framework\TestCase; +use Sabre\HTTP\RequestInterface; +use Sabre\HTTP\ResponseInterface; + +class CorePluginTest extends TestCase { + private CorePlugin $plugin; + public function testGetInfo() { - $corePlugin = new CorePlugin(); - self::assertEquals('core', $corePlugin->getPluginInfo()['name']); + self::assertEquals('core', $this->plugin->getPluginInfo()['name']); + } + + /** + * @dataProvider beforePropertyResolutionEventData + */ + public function testBeforePropertyResolutionEvent(string $basePath, array $originalPropFinds, array $modifiedPropFinds): void + { + $request = $this->createMock(RequestInterface::class); + $request->method('getPath')->willReturn($basePath); + $request->method('getBodyAsString')->willReturn(''); + + $response = $this->createMock(ResponseInterface::class); + + $server = $this->getMockBuilder(Server::class) + ->disableOriginalConstructor() + ->onlyMethods([ + 'getHTTPDepth', + 'getPlugins', + 'getHTTPPrefer', + 'generatePropFindsForPath', + 'getNodePropertiesGenerator', + 'generateMultiStatus', + 'emit', + ]) + ->getMock(); + + $server->method('getHTTPDepth') + ->willReturn(1); + $server->method('getPlugins') + ->willReturn([]); + $server->method('getHTTPPrefer') + ->willReturn(['return' => null]); + + $server->expects($this->once()) + ->method('generatePropFindsForPath') + ->willReturn($originalPropFinds); + + $server->method('generateMultiStatus') + ->willReturn(''); + + $server->method('emit') + ->willReturnCallback( + function (string $eventName, $eventArgs) use ($originalPropFinds, $modifiedPropFinds) { + $this->assertEquals('beforePropertyResolution', $eventName); + /** @var iterable $propFinds */ + [$propFinds] = $eventArgs; + $this->assertIsIterable($propFinds); + $this->assertEquals($originalPropFinds, $propFinds); + $eventArgs[0] = $this->toGenerator($modifiedPropFinds); + + return true; + } + ); + + $server->method('getNodePropertiesGenerator') + ->willReturnCallback(function ($propFinds) use ($modifiedPropFinds) { + // check that the generator received the modified list of PropFinds + $this->assertIsIterable($propFinds); + $array = iterator_to_array($propFinds); + $this->assertEquals($modifiedPropFinds, $array); + + return $this->toGenerator($modifiedPropFinds); + }); + + $server->method('generateMultiStatus') + ->willReturnCallback(function ($fileProperties, $minimal) use ($modifiedPropFinds) { + // check that generateMultiStatus receives the modified list of PropFinds + $array = iterator_to_array($fileProperties); + $this->assertEquals($modifiedPropFinds, $array); + }); + + $this->plugin->initialize($server); + $this->plugin->httpPropFind($request, $response); + } + + private function toGenerator(array $paths): Generator + { + yield from $paths; + } + + /** + * @param string[] $paths + * + * @return array + */ + private function getPropFindNodeTuples(array $paths): array + { + $propFindNodeTuples = []; + foreach ($paths as $path) { + $propFindNodeTuples[] = [ + new PropFind($path, [], 1, PropFind::ALLPROPS), + $this->createMock(INode::class), + ]; + } + + return $propFindNodeTuples; + } + + public function beforePropertyResolutionEventData(): array + { + $basePath = 'files/user'; + $paths = [$basePath]; + for ($i = 0; $i < 5; ++$i) { + $paths[] = "$basePath/$i"; + } + + $originalPropFinds = $this->getPropFindNodeTuples($paths); + $modifiedPropFinds = [$originalPropFinds[0]]; + + return [ + 'Test PROPFIND with all PropFind objects' => [ + $basePath, $originalPropFinds, $originalPropFinds, + ], + 'Test PROPFIND with removed PropFind objects' => [ + $basePath, $originalPropFinds, $modifiedPropFinds, + ], + ]; + } + + protected function setUp(): void + { + parent::setUp(); + $this->plugin = new CorePlugin(); } } From 314fded46cd4aba072abbea873ca8ae67e736705 Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Thu, 23 Oct 2025 17:38:03 +0200 Subject: [PATCH 4/4] test: fix failing tests because of null in ctype function Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- tests/Sabre/CalDAV/CalendarObjectTest.php | 2 +- tests/Sabre/CalDAV/Schedule/SchedulingObjectTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Sabre/CalDAV/CalendarObjectTest.php b/tests/Sabre/CalDAV/CalendarObjectTest.php index 3ac118ce19..4c7cae08fe 100644 --- a/tests/Sabre/CalDAV/CalendarObjectTest.php +++ b/tests/Sabre/CalDAV/CalendarObjectTest.php @@ -117,7 +117,7 @@ public function testGetLastModified() $obj = $children[0]; $lastMod = $obj->getLastModified(); - self::assertTrue(is_int($lastMod) || ctype_digit($lastMod) || is_null($lastMod)); + self::assertTrue(is_null($lastMod) || is_int($lastMod) || ctype_digit($lastMod)); } /** diff --git a/tests/Sabre/CalDAV/Schedule/SchedulingObjectTest.php b/tests/Sabre/CalDAV/Schedule/SchedulingObjectTest.php index 732e6d1703..c7725f62a4 100644 --- a/tests/Sabre/CalDAV/Schedule/SchedulingObjectTest.php +++ b/tests/Sabre/CalDAV/Schedule/SchedulingObjectTest.php @@ -123,7 +123,7 @@ public function testGetLastModified() $obj = $children[0]; $lastMod = $obj->getLastModified(); - self::assertTrue(is_int($lastMod) || ctype_digit($lastMod) || is_null($lastMod)); + self::assertTrue(is_null($lastMod) || is_int($lastMod) || ctype_digit($lastMod)); } /**