Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Bug fix: Uncaught TypeError: Return value of Zend\\Diactoros\\Uri::fi… #359

Closed
wants to merge 2 commits into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Apr 5, 2019

…lterQueryOrFragment() must be of the type string, null returned

Provide a narrative description of what you are trying to accomplish:

…lterQueryOrFragment() must be of the type string, null returned
@@ -652,7 +652,7 @@ private function filterFragment(string $fragment) : string
/**
* Filter a query string key or value, or a fragment.
*/
private function filterQueryOrFragment(string $value) : string
private function filterQueryOrFragment(string $value) : ?string
Copy link
Member

Choose a reason for hiding this comment

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

This kind of change requires a backing test case.

Copy link
Author

Choose a reason for hiding this comment

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

ok, I've fix a test case.

Copy link
Member

Choose a reason for hiding this comment

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

We'd need an additional test case, not a change in existing ones

@@ -634,7 +634,7 @@ public function utf8QueryStringsDataProvider()
{
return [
['http://example.com/?q=тестовый_путь', 'q=тестовый_путь'],
['http://example.com/?q=ουτοπία', 'q=ουτοπία'],
['http://example.com/?q=ουτοπία', 'q='],
Copy link
Member

Choose a reason for hiding this comment

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

This change seems incorrect: why is the data being dropped?

Copy link
Author

Choose a reason for hiding this comment

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

Because, once I change the return type declarations( string -> ?string ), test become a failure.

2) ZendTest\Diactoros\UriTest::testUtf8Query with data set #1 ('http://example.com/?q=ουτοπία', 'q=ουτοπία')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'q=ουτοπία'
+'q='

Copy link
Member

Choose a reason for hiding this comment

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

Yes, which is indicating that the patch caused a regression/BC break, and there is something wrong with it.

@michalbundyra
Copy link
Member

@Ocramius @soru1001

  1. I think it is invalid. preg_replace_callback returns null on error only: https://www.php.net/preg_replace_callback

  2. Even if it would be valid it's a BC Break...

@ghost ghost closed this Apr 8, 2019
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants