Skip to content

Conversation

@amulet1
Copy link
Contributor

@amulet1 amulet1 commented Aug 14, 2025

Highlights:

  • Process nested arrays recursively
  • Convert other values by casting to (string) (this takes care of Horde_Url)
  • Do not output consecutive numeric indexes in arrays
  • Do not skip parameters with empty values
  • Do not encode [ and ]

This also fixes the test failures mentioned in #2.

With this PR, instances of classes without __toString() will not be encoded the way http_build_query would encode them, but this is how it was before 523d5e5, anyway.

More tests should be added, specifically to test nested arrays (possible with Horde_Url), e.g.

    $url->add('x', [ 'abc', 'def', [ 'k1' => 'v1', 'k2' => 'v2 '] ];

@amulet1
Copy link
Contributor Author

amulet1 commented Aug 17, 2025

@TDannhauer Please review.

1 similar comment
@amulet1
Copy link
Contributor Author

amulet1 commented Sep 17, 2025

@TDannhauer Please review.

@amulet1
Copy link
Contributor Author

amulet1 commented Oct 28, 2025

@TDannhauer Any chance it will ever be merged?

@TDannhauer
Copy link
Contributor

hi @amulet1 , sorry for the long delay, I'm back.

regarding your statement "Do not skip parameters with empty values" - can you enlighten me regarding the intened behaviour and how it is achieved? Im not sure if there is a problem

@amulet1
Copy link
Contributor Author

amulet1 commented Nov 29, 2025

There is a difference in how empty string or null values are encoded.

Here is the output of original, current and proposed versions showing the difference:

$params = [ 'a' => 123, 'b' => '', 'c' => null ];
implode('&', $params) http_build_query($params, '', '&') encodeParameters
123&& a=123&b= a=123&b&c

If this is not the desired behavior, it can be easily changed.

Most importantly, the proposed version fixes issue with objects (such as Horde_Url caused by http_build_query), while handilng array indexes the way expected by tests.

@TDannhauer TDannhauer merged commit bf2b51b into horde:FRAMEWORK_6_0 Dec 17, 2025
0 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants