Skip to content

Fixes h() to not return empty string when input string contains invalid code unit sequence.#1080

Closed
uzyn wants to merge 2 commits intocakephp:masterfrom
uzyn:bugfix/debug
Closed

Fixes h() to not return empty string when input string contains invalid code unit sequence.#1080
uzyn wants to merge 2 commits intocakephp:masterfrom
uzyn:bugfix/debug

Conversation

@uzyn
Copy link
Copy Markdown
Contributor

@uzyn uzyn commented Jan 23, 2013

Caught me by surprise when I passed an object containing some invalid code unit sequence (binary data) to debug() but it returned me an empty string, even though there are plaintext properties (which I was interested in).

Even for normal usage, I think h() should not return a result that's far too different from the original input, especially when there are other valid plantext characters.

@dereuromark
Copy link
Copy Markdown
Member

I think I ran into the same thing a few times, as well. Wondering why the non-empty string was "supposedly" empty in debug output.

@majna
Copy link
Copy Markdown
Contributor

majna commented Jan 23, 2013

ENT_SUBSTITUTE is introduced in PHP 5.4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Better to use the cake.power.gif image from Test/test_app/. Also as per cake's coding standard there should be a space before and after each .. You can use https://github.com/cakephp/cakephp-codesniffer

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Coincidentally, I have also just submitted a better test case without having to rely on binary file.

@uzyn
Copy link
Copy Markdown
Contributor Author

uzyn commented Jan 23, 2013

@majna You're right. I did not realize that.

Even ENT_IGNORE is only available from 5.3.0 while CakePHP 2.x supports PHP >= 5.2.8.

I guess we'll have to run a regex and replace the invalid chars manually.

@lorenzo
Copy link
Copy Markdown
Member

lorenzo commented Jan 23, 2013

You can target your PR for 3.0, though :)

@markstory
Copy link
Copy Markdown
Member

Closing as this requires features that don't exist in 5.2.

@markstory markstory closed this Jan 23, 2013
@markstory
Copy link
Copy Markdown
Member

If you don't mind the extra work a PR for 3.0 would be greatly appreciated :D

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.

6 participants