Skip to content
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.

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

Strings: throw exception on malformed UTF-8 in webalize() and toAscii() #205

Merged
merged 1 commit into from Dec 3, 2019

Conversation

VasekPurchart
Copy link
Contributor

@VasekPurchart VasekPurchart commented Dec 2, 2019

  • bug fix
  • BC break? no

Since Strings::webalize() can be called on user input it should be able to deal with anything or error in a predicable way. But when an malformed UTF-8 input is given currently it results in:

Nette\Utils\RegexpException was expected but got TypeError (strtr() expects parameter 1 to be string, null given)

(i guess this behaved differently before scrict types were added in 3.0.0).

Throwing an exception (the same exception as in other cases such as Strings::replace()) seems reasonable to me.

I am also not sure why the internal handling of PCRE is not used everywhere in calling preg_* is not used everywhere in the file, because there might be of course other problems too?

@VasekPurchart
Copy link
Contributor Author

@VasekPurchart VasekPurchart commented Dec 2, 2019

Travis failures seem to be unrelated to this PR.

@dg
Copy link
Member

@dg dg commented Dec 3, 2019

Thanks

@dg dg merged commit aa00974 into nette:master Dec 3, 2019
1 check failed
@VasekPurchart VasekPurchart deleted the webalize-malformed-utf branch Dec 3, 2019
@VasekPurchart
Copy link
Contributor Author

@VasekPurchart VasekPurchart commented Dec 3, 2019

@dg Thanks for merging. As for the other places in Strings where the native preg_ functions are called - do you want them changed as well?

@dg
Copy link
Member

@dg dg commented Dec 3, 2019

I did it 6339ca2

@VasekPurchart
Copy link
Contributor Author

@VasekPurchart VasekPurchart commented Dec 3, 2019

Great, thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants