Skip to content

T dannhauer additional tests for horde/url #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: FRAMEWORK_6_0
Choose a base branch
from
Open

T dannhauer additional tests for horde/url #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a888d82
Create HordeUrlParamRawModeTest.php
TDannhauer Aug 13, 2025
fe9b5bd
Create NestedParamHordeUrlTest.php
TDannhauer Aug 13, 2025
d574b89
Create StringableParamTest.php
TDannhauer Aug 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions test/Horde/Url/HordeUrlParamRawModeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php
/**
* @author Torben Dannhauer / GPT-5
* @license http://www.horde.org/licenses/lgpl21 LGPL 2.1
* @category Horde
* @package Url
* @subpackage UnitTests
*/

namespace Horde\Url;
use \PHPUnit\Framework\TestCase;
use \Horde_Url;

class HordeUrlParamRawModeTest extends TestCase
{
public function testHordeUrlParamEscapedAndRaw()
{
$url = new Horde_Url('test');
$url->add('url', new Horde_Url('https://example.com/test?_t=123456&_h=Abcd123'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add something else here to see the difference with ampersands?

        $url->add('x', 123);

$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
);
}
Copy link

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method uses tabs for indentation which is inconsistent with PSR-12 coding standards that recommend 4 spaces.

Suggested change
}
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.
}


30 changes: 30 additions & 0 deletions test/Horde/Url/NestedParamHordeUrlTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php
/**
* @author Torben Dannhauer / GPT-5
* @license http://www.horde.org/licenses/lgpl21 LGPL 2.1
* @category Horde
* @package Url
* @subpackage UnitTests
*/

namespace Horde\Url;
use \PHPUnit\Framework\TestCase;
use \Horde_Url;

class NestedParamHordeUrlTest extends TestCase
{
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$this->markTestIncomplete('Recursive normalization of nested Horde_Url parameters is not implemented yet.');
}
Copy link

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method uses tabs for indentation which is inconsistent with PSR-12 coding standards that recommend 4 spaces.

Suggested change
}
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.
}


33 changes: 33 additions & 0 deletions test/Horde/Url/StringableParamTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php
/**
* @author Torben Dannhauer / GPT-5
* @license http://www.horde.org/licenses/lgpl21 LGPL 2.1
* @category Horde
* @package Url
* @subpackage UnitTests
*/

namespace Horde\Url;
use \PHPUnit\Framework\TestCase;
use \Horde_Url;

class StringableParamTest extends TestCase
{
private function getStringableObject($value)
{
return new class($value) {
private $value;
public function __construct($value) { $this->value = $value; }
public function __toString() { return (string)$this->value; }
};
}
Copy link

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method uses tabs for indentation which is inconsistent with PSR-12 coding standards that recommend 4 spaces.

Suggested change
}
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.

public function testStringableObjectAsParamValue()
{
$this->markTestIncomplete('Generalized normalization for all stringable objects may be implemented later.');
$url = new Horde_Url('test');
$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.
}
Copy link
Contributor

@amulet1 amulet1 Aug 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

}
Loading