T dannhauer additional tests for horde/url #3
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds additional test coverage for the Horde URL library, specifically focusing on parameter handling edge cases. The tests cover stringable objects as parameters, nested arrays containing Horde_Url instances, and the behavior of Horde_Url parameters in both escaped and raw output modes.
- Adds test for handling stringable objects as URL parameters (marked as incomplete)
- Adds test for nested array parameters containing Horde_Url instances (marked as incomplete)
- Adds test for Horde_Url parameter behavior in raw vs escaped output modes
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/Horde/Url/StringableParamTest.php | Tests stringable object handling as URL parameters |
| test/Horde/Url/NestedParamHordeUrlTest.php | Tests nested array parameters containing Horde_Url instances |
| test/Horde/Url/HordeUrlParamRawModeTest.php | Tests Horde_Url parameter behavior in raw vs escaped modes |
Comments suppressed due to low confidence (2)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| public function __construct($value) { $this->value = $value; } | ||
| public function __toString() { return (string)$this->value; } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Method uses tabs for indentation which is inconsistent with PSR-12 coding standards that recommend 4 spaces.
| } | |
| private function getStringableObject($value) | |
| { | |
| return new class($value) { | |
| private $value; | |
| public function __construct($value) { $this->value = $value; } | |
| public function __toString() { return (string)$this->value; } | |
| }; | |
| } |
Copilot uses AI. Check for mistakes.
| // This test documents current behavior and should be adapted once recursion is implemented. | ||
| $this->assertStringContainsString('outer%5Binner%5D%5Bparameters%5D', (string)$url); | ||
| $this->markTestIncomplete('Recursive normalization of nested Horde_Url parameters is not implemented yet.'); | ||
| } |
There was a problem hiding this comment.
Method uses tabs for indentation which is inconsistent with PSR-12 coding standards that recommend 4 spaces.
| } | |
| public function testNestedArrayWithHordeUrlIsCurrentlyNotConvertedRecursively() | |
| { | |
| $url = new Horde_Url('test'); | |
| $url->add('outer', [ | |
| 'inner' => new Horde_Url('https://example.com/test?_t=1&_h=2') | |
| ]); | |
| // Current behavior: Only top-level values are normalized by PR #2. | |
| // Nested arrays containing Horde_Url will still be expanded by http_build_query. | |
| // This test documents current behavior and should be adapted once recursion is implemented. | |
| $this->assertStringContainsString('outer%5Binner%5D%5Bparameters%5D', (string)$url); | |
| $this->markTestIncomplete('Recursive normalization of nested Horde_Url parameters is not implemented yet.'); | |
| } |
Copilot uses AI. Check for mistakes.
| 'test?url=https%3A%2F%2Fexample.com%2Ftest%3F_t%3D123456%26_h%3DAbcd123', | ||
| (string)$url | ||
| ); | ||
| } |
There was a problem hiding this comment.
Method uses tabs for indentation which is inconsistent with PSR-12 coding standards that recommend 4 spaces.
| } | |
| public function testHordeUrlParamEscapedAndRaw() | |
| { | |
| $url = new Horde_Url('test'); | |
| $url->add('url', new Horde_Url('https://example.com/test?_t=123456&_h=Abcd123')); | |
| $this->assertEquals( | |
| 'test?url=https%3A%2F%2Fexample.com%2Ftest%3F_t%3D123456%26_h%3DAbcd123', | |
| (string)$url | |
| ); | |
| // Raw output should not HTML-escape the ampersands | |
| $url->setRaw(true); | |
| $this->assertEquals( | |
| 'test?url=https%3A%2F%2Fexample.com%2Ftest%3F_t%3D123456%26_h%3DAbcd123', | |
| (string)$url | |
| ); | |
| } |
Copilot uses AI. Check for mistakes.
| public function testHordeUrlParamEscapedAndRaw() | ||
| { | ||
| $url = new Horde_Url('test'); | ||
| $url->add('url', new Horde_Url('https://example.com/test?_t=123456&_h=Abcd123')); |
There was a problem hiding this comment.
Maybe we should add something else here to see the difference with ampersands?
$url->add('x', 123);| // Current behavior: Only top-level values are normalized by PR #2. | ||
| // Nested arrays containing Horde_Url will still be expanded by http_build_query. | ||
| // This test documents current behavior and should be adapted once recursion is implemented. | ||
| $this->assertStringContainsString('outer%5Binner%5D%5Bparameters%5D', (string)$url); |
There was a problem hiding this comment.
If needed, it is not hard to modify the code to support objects at deeper levels.
The 523d5e5 commit broke object-to-string conversion (in particular, the Horde_Url case) while also added support for nested arrays.
To properly fix it, we need to agree on which object(s) exactly we are converting to string [using strval() or (string)]. Currently the fix is specifically for Horde_Url class. If it is not only Horde_Url, we could use a hardcoded list of class names, or check if __toString() method is defined. Both ways have some cons and pros.
Also, an alternative approach might be to check/convert objects passed to add method in $value parameter instead of doing it in toString() method. This might work if $parameters is only used internally.
| $url->add('s', $this->getStringableObject('a&b')); | ||
| // Current implementation only normalizes Horde_Url instances; generic stringables are not cast explicitly. | ||
| // Keep test incomplete until generalized handling is agreed upon. | ||
| } |
There was a problem hiding this comment.
Do we want to make it truly universal? Then I guess we need to check for presence of __toString method (see my other comments also). Are we okay with using ReflectionClass::hasMethodor method_exists (instead of explicit conversion)?
|
Please see #4. |
Additional tests prepared