Skip to content

it should enable --enable-mbstring during compilation #34

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

Closed
docteurklein opened this issue Nov 6, 2014 · 26 comments
Closed

it should enable --enable-mbstring during compilation #34

docteurklein opened this issue Nov 6, 2014 · 26 comments

Comments

@docteurklein
Copy link

No description provided.

@md5
Copy link
Contributor

md5 commented Nov 7, 2014

Do you think this needs to be enabled by default or would it be fine to have to add an extension=mbstring.so to the php.ini file?

@tianon tianon mentioned this issue Nov 7, 2014
@thaJeztah
Copy link
Contributor

Contrary to my other comments wrt enabling / disabling extensions, I think this extension should be enabled by default.

Using UTF-8 has become the standard on the internet. Not using UTF-8 will be a rare exception, so I think that using mbstring is the right decision here.

Even further, I think that overloading (mbstring.func_overload string) should also be enabled with this so that most string functions will work properly with UTF-8 strings, without the need to explicitly call their mb_xxxx counterparts.

@docteurklein
Copy link
Author

mbstring being a non-default extension, it's mandatory to recompile php with the flag enabled in order to activate it.

Unless I'm wrong, there is no .so to register (at least not in my php.ini).

@docteurklein docteurklein changed the title it should enable --with-mbstring during compilation it should enable --enable-mbstring during compilation Nov 10, 2014
@md5
Copy link
Contributor

md5 commented Nov 10, 2014

The only reason you don't have mbstring.so is that yours is compiled in statically. For what it's worth, I think all the extensions we're talking about having to build for this image are "non-standard".

Using this POC commit, it's possible to build mbstring as a shared module in a separate build from the main PHP source. Here's an example of using it, lightly modified from the mbstring docs:

mike@Hrothgar:~$ cat example2.php 
<?php
$str = "Τάχιστη αλώπηξ βαφής ψημένη γη, δρασκελίζει υπέρ νωθρού κυνός";
$str = mb_convert_case($str, MB_CASE_UPPER, "UTF-8");
echo "$str\n"; // Prints ΤΆΧΙΣΤΗ ΑΛΏΠΗΞ ΒΑΦΉΣ ΨΗΜΈΝΗ ΓΗ, ΔΡΑΣΚΕΛΊΖΕΙ ΥΠΈΡ ΝΩΘΡΟΎ ΚΥΝΌΣ
$str = mb_convert_case($str, MB_CASE_TITLE, "UTF-8");
echo "$str\n"; // Prints Τάχιστη Αλώπηξ Βαφήσ Ψημένη Γη, Δρασκελίζει Υπέρ Νωθρού Κυνόσ
?>
mike@Hrothgar:~$ docker run -it --rm -v $(pwd)/example2.php:/example2.php php:mbstring php -dextension=mbstring.so /example2.php
ΤΆΧΙΣΤΗ ΑΛΏΠΗΞ ΒΑΦΉΣ ΨΗΜΈΝΗ ΓΗ, ΔΡΑΣΚΕΛΊΖΕΙ ΥΠΈΡ ΝΩΘΡΟΎ ΚΥΝΌΣ
Τάχιστη Αλώπηξ Βαφήσ Ψημένη Γη, Δρασκελίζει Υπέρ Νωθρού Κυνόσ

@md5
Copy link
Contributor

md5 commented Nov 10, 2014

That being said, I actually do agree that mbstring should be compiled as static by default.

@docteurklein
Copy link
Author

ho, that's good to know! thanks for the feeback.

@md5
Copy link
Contributor

md5 commented Nov 10, 2014

@thaJeztah while I like the idea of encouraging new apps to use that default, I fear that making that the default for this image will break in the cases where someone is just trying to Dockerize an old PHP app (ahem, like me). If that app was not written to expect that override turned on by default, it seems like the app could break in subtle ways that might not be caught during initial testing (e.g. if i18n checks are not part of that initial testing).

@docteurklein
Copy link
Author

@md5 are you talking about mbstring.func_overload ?

I also think this option should not be activated by default, since it would potentially break expected behavior.

@md5
Copy link
Contributor

md5 commented Nov 10, 2014

Yes, that's what I was talking about.

@thaJeztah
Copy link
Contributor

In all cases, mbstring.func_overload is something to be set in php.ini. Since this image doesn't have a php.ini, enabling/disabling it by default is not an issue anyway 😄

@md5
Copy link
Contributor

md5 commented Nov 10, 2014

@thaJeztah I thought you were suggesting that the image add a default php.ini with that setting turned on.

@thaJeztah
Copy link
Contributor

lol yes, should have read back my earlier comment, making a total fool of myself here 😄

I thought enabling overloading by default would make sense, but perhaps you're right; when working on legacy projects it would be surprising behavior.

I think I'm fine either way; both choices have their pros and cons. If we stay with the current setup (no php.ini) that's ok.

@ka2er
Copy link

ka2er commented Nov 18, 2014

+1 I would really appreciate having mbstring extension compiled by default

@docteurklein
Copy link
Author

In fact it's unfortunalty required by some libraries such as https://github.com/videlalvaro/php-amqplib, which forces me to use a non-official image (such as jolicode/php5*).

@tianon
Copy link
Member

tianon commented Nov 19, 2014

This seems to work here: (I don't have RabbitMQ handy to test connecting through, but the demos do run and give back Error Connecting to server (111): Connection refused)

FROM php

RUN apt-get update && apt-get install -y git

RUN curl -sS https://getcomposer.org/installer | php -- --install-dir=/usr/local/bin

RUN docker-php-ext-install bcmath mbstring

RUN git clone https://github.com/videlalvaro/php-amqplib.git /usr/src/amqplib
WORKDIR /usr/src/amqplib

RUN composer.phar install

@docteurklein
Copy link
Author

ha, I haven't seen this docker-php-ext-install thing yet!
I guess it resolves the problem then.

Nevertheless, having mbstring without having to make a custom image would be cool.

@bocharsky-bw
Copy link

I voting 👍 for mbstring out-of-the-box.

@anroots
Copy link

anroots commented Jun 24, 2015

Support mbstring out of the box.

Fatal error: Call to undefined function Illuminate\Foundation\Bootstrap\mb_internal_encoding() in /var/www/html/vendor/compiled.php on line 1809

@iammichiel
Copy link

No update on this? As many others, I would like to see this enabled by default.

@yosifkit
Copy link
Member

We have to draw the line of what to include by default and currently that line is "what must be compiled into php itself versus what can be added after". *following upstreams recommendations, of course.

@macropin
Copy link

@yosifkit With a CentOS container, if I need mbstring, I can simply go yum -y install php-mbstring and I've got it. If I use this official language container then I'm so out of luck without resorting to rebuilding php in the container which defeats the purpose of official language containers.

Please find a way to allow this module (and others) to be enabled. Because without it this container is not usable.

@md5
Copy link
Contributor

md5 commented Aug 24, 2015

@macropin It isn't necessary to rebuild php in the container. I'm pretty sure it's been established that the mbstring extension can be installed like this:

FROM php
RUN docker-php-ext-install mbstring

I don't see any comments here that contradict that and my test from November 2014 seemed to work: #34 (comment)

@macropin
Copy link

@md5 thanks. I didn't think that script was finalised. I didn't see any docs for it.

Does this mean that the container will always be shipped with php sources included?

@md5
Copy link
Contributor

md5 commented Aug 25, 2015

@macropin The docs are unfortunately being truncated on Docker Hub, but you can read them here: https://github.com/docker-library/docs/blob/master/php/README.md#how-to-install-more-php-extensions

The docker-php-ext-* scripts were added in November 2014 and haven't undergone much change since then.

To answer your question, yes the container will always be shipped with the PHP source code.

@mathroc
Copy link
Contributor

mathroc commented Jan 14, 2016

👍 I'd like to have mbstring already installed too

with php7+ I think it could be enabled by default and with mbstring.func_overload set to 4

@TimVervers
Copy link

Voting for mbstring already installed too!

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

No branches or pull requests