Removed substr() and strlen() in favor of mb_*

[ezimuel]

This PR fixes the potential issue reported in #20.

[ezimuel]

@weierophinney can you merge this? Thanks!

[weierophinney]

Question: what happens when ext/mbstring is unavailable? Does the component throw an exception?

[ezimuel]

@weierophinney no it doesn't :( We should add the ext/mbstring dependency on composer, without managing it in the code, what do you think?

[weierophinney]

@ezimuel that would work. Add that, and then I think I can merge. I'll merge to develop for a forthcoming minor and/or major release, as it adds a requirement to the package.

[weierophinney]

Merged to develop for release with either a 2.7.0 or 3.0.0 release.

[Ocramius]

@weierophinney patch had no tests :-(

[ezimuel]

@Ocramius adding a test here is like to test the PHP str functions for binary safe. See this issue ezimuel/phpcrypto#4. I don't think we need to provide one here. Thoughts?

[Ocramius]

Disagreement here. Anybody patching code that isn't tested against
multibyte strings could potentially re-introduce the problem. Also, there
is no validation for this fix.
On Apr 26, 2016 16:45, "Enrico Zimuel" notifications@github.com wrote:

@Ocramius https://github.com/Ocramius adding a test here is like to
test the PHP str functions for binary safe. See this issue
ezimuel/phpcrypto#4 ezimuel/phpcrypto#4. I
don't think we need to provide one here. Thoughts?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#22 (comment)

[ezimuel]

@Ocramius ok, but how we can prevent to re-introduce the problem here? We should test all the functions of our code with a multibyte string?

[weierophinney]

Oof, yeah, I see your point, @Ocramius ; I was looking at it primarily as an internal refactor, and tests continued to pass, but the point is that multibyte strings should now also work, and if they don't it would be indicative of a regression.

@ezimuel — this would mean adding either a new test case, or an element in a test provider, for each of the methods affected. Let's open an issue for that, and schedule it for a 2.7.1 release.

[ezimuel]

@weierophinney I just opened the issue #23 /cc @Ocramius

[Ocramius]

Thanks, @weierophinney @ezimuel

I know I'm a pain in the ar_cough_, I am just worried about the actual issue though :-)