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

Fix for mb function overload mb_substr acting different #1453

Closed
wants to merge 3 commits into from
Closed

Fix for mb function overload mb_substr acting different #1453

wants to merge 3 commits into from

Conversation

1emming
Copy link
Contributor

@1emming 1emming commented Jul 27, 2014

When bug hunting https://github.com/fabpot/Twig/issues/1428 I ran into an issue.
I started out by changing my php.ini for the cli enviroment, I've added:
mbstring.func_overload = 2
Later I tested with adding and removing:
mbstring.internal_encoding = 'UTF-8'

It turns out mb_substr returns false and not '' when doing something like this:

$item = '';
$start = -1;
$length = 1;
$charset = 'UTF-8';
$a = mb_substr($item, $start, $length, $charset);
var_dump($a);
die;

I'm not sure this will fix 1428 since I don't have enough information for this, but this PR should at least fix one issue.

@fabpot
Copy link
Contributor

fabpot commented Jul 28, 2014

Is it possible to add a test that covers this issue?

@1emming
Copy link
Contributor Author

1emming commented Jul 28, 2014

When setting the option in /test/bootstrap.php:

ini_set('mbstring.func_overload', '2');

all goes well. When setting it directly in the php.ini it goes wrong (the Intergration tests for first and last fail). This is really and issue with mbstring, but changes to that are slow :/

(While odd this also seems the case for some users running Windows & Apache2 who try to set it the .htaccess (doesn't work either @see http://php.net/manual/en/mbstring.overload.php).)

Therefore I cannot provide a test since it only happends when setting it in the php.ini file. I've tried it on Ubuntu and Debian and in both cases it goes wrong. Could you try running the test (on master) with the change in your php.ini ?

@@ -702,7 +702,12 @@ function twig_slice(Twig_Environment $env, $item, $start, $length = null, $prese
$item = (string) $item;

if (function_exists('mb_get_info') && null !== $charset = $env->getCharset()) {
return mb_substr($item, $start, null === $length ? mb_strlen($item, $charset) - $start : $length, $charset);
$r = mb_substr($item, $start, null === $length ? mb_strlen($item, $charset) - $start : $length, $charset);
if (false === $r) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about just casting to a string? return (string) mb_substr(...);

@1emming
Copy link
Contributor Author

1emming commented Jul 28, 2014

@fabpot I agree on the cast over the result check, please last commit :)

@fabpot
Copy link
Contributor

fabpot commented Jul 28, 2014

A small thing: you inadvertently changed the file rights to 0755, can you revert this change?

@fabpot
Copy link
Contributor

fabpot commented Jul 28, 2014

Thank you @1emming.

@fabpot fabpot closed this Jul 28, 2014
@fabpot fabpot closed this in 7d1577f Jul 28, 2014
@1emming 1emming deleted the Fix_mb_func_overload branch August 22, 2014 16:47
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