From 4acfe9d1d84493ea90ba86d390f41ed1c01fffb6 Mon Sep 17 00:00:00 2001 From: Ed Wilde Date: Thu, 17 Oct 2024 10:17:12 +1300 Subject: [PATCH 1/3] FIX Make sure the `href` attribute is constructed correctly when trailing slash is used Prior to the fix, when trailing slash is enabled the url output is incorrect; the second test fails because the href output contains: `/api/v1/SilverStripe-ORM-DataObject/1/.xml`. --- src/DataFormatter/XMLDataFormatter.php | 4 +- tests/unit/XMLDataFormatterTest.php | 69 ++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/src/DataFormatter/XMLDataFormatter.php b/src/DataFormatter/XMLDataFormatter.php index c4ba879..5b8c559 100644 --- a/src/DataFormatter/XMLDataFormatter.php +++ b/src/DataFormatter/XMLDataFormatter.php @@ -117,9 +117,9 @@ public function convertDataObjectWithoutHeader(DataObject $obj, $fields = null, { $className = $this->sanitiseClassName(get_class($obj)); $id = $obj->ID; - $objHref = Director::absoluteURL($this->config()->api_base . "$className/$obj->ID"); + $objHref = Director::absoluteURL($this->config()->api_base . "$className/$obj->ID" . ".xml"); - $xml = "<$className href=\"$objHref.xml\">\n"; + $xml = "<$className href=\"$objHref\">\n"; foreach ($this->getFieldsForObj($obj) as $fieldName => $fieldType) { // Field filtering if ($fields && !in_array($fieldName, $fields ?? [])) { diff --git a/tests/unit/XMLDataFormatterTest.php b/tests/unit/XMLDataFormatterTest.php index 8da70a1..328068a 100644 --- a/tests/unit/XMLDataFormatterTest.php +++ b/tests/unit/XMLDataFormatterTest.php @@ -5,6 +5,8 @@ use SilverStripe\Dev\SapphireTest; use SilverStripe\RestfulServer\DataFormatter\XMLDataFormatter; use Exception; +use SilverStripe\Control\Controller; +use SilverStripe\ORM\DataObject; class XMLDataFormatterTest extends SapphireTest { @@ -74,4 +76,71 @@ public function testConvertStringToArrayMultipleEntitiesException() $formatter = new XMLDataFormatter(); $formatter->convertStringToArray($inputXML); } + + /** + * Tests wrapper output of {@link XMLDataFormatter::convertDataObjectWithoutHeader()} + */ + public function testConvertDataObjectWithoutHeaderClassNameAttribute(): void + { + // Create a mock object + $mock = DataObject::create(); + $mock->ID = 1; + + // Disable trailing slash by default + Controller::config()->set('add_trailing_slash', false); + + // Create a formatter + $formatter = new XMLDataFormatter(); + + // Test the output + $expectedClass = 'SilverStripe-ORM-DataObject'; + $expectedHref = sprintf('http://localhost/api/v1/%s/%d.xml', $expectedClass, $mock->ID); + $expectedOutput = sprintf( + '<%s href="%s">%d', + $expectedClass, + $expectedHref, + $mock->ID, + $expectedClass + ); + + $actualOutput = $formatter->convertDataObjectWithoutHeader($mock); + + // remove line breaks and compare + $actualOutput = str_replace(["\n", "\r"], '', $actualOutput); + $this->assertEquals($expectedOutput, $actualOutput); + } + + /** + * Tests wrapper output of {@link XMLDataFormatter::convertDataObjectWithoutHeader()} when + * used with a forced trailing slash + */ + public function testConvertDataObjectWithoutHeaderClassNameAttributeWithTrailingSlash(): void + { + // Create a mock object + $mock = DataObject::create(); + $mock->ID = 1; + + // Enable trailing slash by default + Controller::config()->set('add_trailing_slash', true); + + // Create a formatter + $formatter = new XMLDataFormatter(); + + // Test the output + $expectedClass = 'SilverStripe-ORM-DataObject'; + $expectedHref = sprintf('http://localhost/api/v1/%s/%d.xml', $expectedClass, $mock->ID); + $expectedOutput = sprintf( + '<%s href="%s">%d', + $expectedClass, + $expectedHref, + $mock->ID, + $expectedClass + ); + + $actualOutput = $formatter->convertDataObjectWithoutHeader($mock); + + // remove line breaks and compare + $actualOutput = str_replace(["\n", "\r"], '', $actualOutput); + $this->assertEquals($expectedOutput, $actualOutput); + } } From 83cbb59c722dc91f4fa01df46c8b89a9957ed302 Mon Sep 17 00:00:00 2001 From: Ed Wilde Date: Thu, 17 Oct 2024 12:04:03 +1300 Subject: [PATCH 2/3] FIX Apply the same fix to other usage of `absoluteURL()` --- src/DataFormatter/XMLDataFormatter.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/DataFormatter/XMLDataFormatter.php b/src/DataFormatter/XMLDataFormatter.php index 5b8c559..db91c34 100644 --- a/src/DataFormatter/XMLDataFormatter.php +++ b/src/DataFormatter/XMLDataFormatter.php @@ -160,11 +160,11 @@ public function convertDataObjectWithoutHeader(DataObject $obj, $fields = null, $fieldName = $relName . 'ID'; if ($obj->$fieldName) { - $href = Director::absoluteURL($this->config()->api_base . "$relClass/" . $obj->$fieldName); + $href = Director::absoluteURL($this->config()->api_base . "$relClass/" . $obj->$fieldName . ".xml"); } else { - $href = Director::absoluteURL($this->config()->api_base . "$className/$id/$relName"); + $href = Director::absoluteURL($this->config()->api_base . "$className/$id/$relName" . ".xml"); } - $xml .= "<$relName linktype=\"has_one\" href=\"$href.xml\" id=\"" . $obj->$fieldName + $xml .= "<$relName linktype=\"has_one\" href=\"$href\" id=\"" . $obj->$fieldName . "\">\n"; } @@ -190,8 +190,8 @@ public function convertDataObjectWithoutHeader(DataObject $obj, $fields = null, $items = $obj->$relName(); if ($items) { foreach ($items as $item) { - $href = Director::absoluteURL($this->config()->api_base . "$relClass/$item->ID"); - $xml .= "<$relClass href=\"$href.xml\" id=\"{$item->ID}\">\n"; + $href = Director::absoluteURL($this->config()->api_base . "$relClass/$item->ID" . ".xml"); + $xml .= "<$relClass href=\"$href\" id=\"{$item->ID}\">\n"; } } $xml .= "\n"; @@ -221,8 +221,8 @@ public function convertDataObjectWithoutHeader(DataObject $obj, $fields = null, $items = $obj->$relName(); if ($items) { foreach ($items as $item) { - $href = Director::absoluteURL($this->config()->api_base . "$relClass/$item->ID"); - $xml .= "<$relClass href=\"$href.xml\" id=\"{$item->ID}\">\n"; + $href = Director::absoluteURL($this->config()->api_base . "$relClass/$item->ID" . ".xml"); + $xml .= "<$relClass href=\"$href\" id=\"{$item->ID}\">\n"; } } $xml .= "\n"; From 573f4bf3540fc826b2c9f69102971a29192f4478 Mon Sep 17 00:00:00 2001 From: Ed Wilde Date: Wed, 23 Oct 2024 08:31:03 +1300 Subject: [PATCH 3/3] FIX Refactor test to use a data provider and remove duplication --- tests/unit/XMLDataFormatterTest.php | 46 ++++++++--------------------- 1 file changed, 13 insertions(+), 33 deletions(-) diff --git a/tests/unit/XMLDataFormatterTest.php b/tests/unit/XMLDataFormatterTest.php index 328068a..9b3ffe1 100644 --- a/tests/unit/XMLDataFormatterTest.php +++ b/tests/unit/XMLDataFormatterTest.php @@ -78,50 +78,30 @@ public function testConvertStringToArrayMultipleEntitiesException() } /** - * Tests wrapper output of {@link XMLDataFormatter::convertDataObjectWithoutHeader()} + * Data provider for trailing slash configuration */ - public function testConvertDataObjectWithoutHeaderClassNameAttribute(): void + public function trailingSlashProvider(): array { - // Create a mock object - $mock = DataObject::create(); - $mock->ID = 1; - - // Disable trailing slash by default - Controller::config()->set('add_trailing_slash', false); - - // Create a formatter - $formatter = new XMLDataFormatter(); - - // Test the output - $expectedClass = 'SilverStripe-ORM-DataObject'; - $expectedHref = sprintf('http://localhost/api/v1/%s/%d.xml', $expectedClass, $mock->ID); - $expectedOutput = sprintf( - '<%s href="%s">%d', - $expectedClass, - $expectedHref, - $mock->ID, - $expectedClass - ); - - $actualOutput = $formatter->convertDataObjectWithoutHeader($mock); - - // remove line breaks and compare - $actualOutput = str_replace(["\n", "\r"], '', $actualOutput); - $this->assertEquals($expectedOutput, $actualOutput); + return [ + 'without trailing slash' => [false], + 'with trailing slash' => [true], + ]; } /** - * Tests wrapper output of {@link XMLDataFormatter::convertDataObjectWithoutHeader()} when - * used with a forced trailing slash + * Tests wrapper output of {@link XMLDataFormatter::convertDataObjectWithoutHeader()} + * + * @param bool $addTrailingSlash - Whether to add a trailing slash to the API endpoint + * @dataProvider trailingSlashProvider */ - public function testConvertDataObjectWithoutHeaderClassNameAttributeWithTrailingSlash(): void + public function testConvertDataObjectWithoutHeaderClassNameAttribute(bool $addTrailingSlash): void { // Create a mock object $mock = DataObject::create(); $mock->ID = 1; - // Enable trailing slash by default - Controller::config()->set('add_trailing_slash', true); + // Set trailing slash configuration + Controller::config()->set('add_trailing_slash', $addTrailingSlash); // Create a formatter $formatter = new XMLDataFormatter();