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

Removing mbstring dependency #541

Closed
wants to merge 5 commits into from
Closed

Conversation

skratte
Copy link

@skratte skratte commented Nov 19, 2017

The dependency to mbstring in php7 could be removed with a fallback to strlen.

The dependency to mbstring in php7 could be removed with a fallback to strlen.
@cebe
Copy link
Contributor

cebe commented Nov 20, 2017

Could you provide a source for that?

According to the documentation your change is wrong:

strlen() returns the number of bytes rather than the number of characters in a string.
http://php.net/manual/en/function.strlen.php

@PhrozenByte
Copy link
Contributor

How about using https://github.com/symfony/polyfill-mbstring?

@skratte
Copy link
Author

skratte commented Nov 23, 2017

You are right. I was too careless and didn't see the differences in the results.

I did a little more research on this and the code could be like this:


                foreach ($parts as $part)
                {
                    if(function_exists('mb_strlen'))
                    {   
                        $shortage = 4 - mb_strlen($line, 'UTF-8') % 4;
                    }
                    elseif(function_exists('iconv_strlen'))
                    {   
                        $shortage = 4 - @iconv_strlen($line, 'UTF-8') % 4;
                    }
                    else
                    {
                        $shortage = 4 - preg_match_all("/.{1}/us",$line) % 4;
                    }

                    $line .= str_repeat(' ', $shortage);
                    $line .= $part;
                }

This way it will fallback from non-default extension to default extension to core extension.

As stated in a comment to iconv_strlen, it will fail with badly formatted sequences. Because of this i had to silence iconv_strlen.

With non of the solutions i get the expected results on bad sequences. To test bad sequences i used this: http://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-test.txt

All in all, the code is not pretty but it works as it should.

@aidantwoods
Copy link
Collaborator

mb_string is now explicitly required in composer.json.

There's some discussion in #561, but to sum it up:

Seeing as how all alternative implementations use extensions themselves, there isn't really a way to implement a fallback without depending on at least one extension.

Even if relying on a default extension, we should still declare this dependence (as default extensions can be disabled), so we might as well just depend on the extension that does what we need directly. And if people want to install polyfills that depend on other extensions, then they can still do that with the added knowledge of which extensions they have available.

Hope that all makes sense :)

@PhrozenByte
Copy link
Contributor

PhrozenByte commented Mar 9, 2018

Just one small addition:

If you're developing a end-user application with Parsedown, and if you don't want to require your users to install ext-mbstring, simply add the following to your composer.json:

{
    "require": {
        "erusev/parsedown": "^1.7.1",
        "symfony/polyfill-mbstring": "^1.7"
    },
    "config": {
        "platform": { "ext-mbstring": "7" }
    }
}

However, please be warned that this isn't recommended (quite the contrary). You should rather tell your users to install ext-mbstring, the polyfill is very slow.

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.

None yet

4 participants