markshust / docker-magento2-php

This image is built from the official php repository and contains PHP configurations for Magento 2.
MIT License
52 stars 61 forks source link

Fix invalid environment variable dereference in php.ini #27

Closed hamdrew closed 8 years ago

hamdrew commented 8 years ago

This pull request fixes a bug pertaining to the memory_limit directive in conf/php.ini. It was incorrectly dereferencing the _PHP_MEMORYLIMIT environment variable.

This bug causes the following error to appear when running any sizable PHP script:

Fatal error: Allowed memory size of 2097152 bytes exhausted

Related output from php -i before this change:

memory_limit => PHP_MEMORY_LIMIT => PHP_MEMORY_LIMIT
PHP_MEMORY_LIMIT => 2G

And after this change:

memory_limit => 2G => 2G
PHP_MEMORY_LIMIT => 2G

This change should remove the need to specify -d memory_limit=2G in bin/mage-setup-raw.

hamdrew commented 8 years ago

Source: http://php.net/manual/en/configuration.file.php (See Example 1)

markshust commented 8 years ago

I don't believe this is needed, because we're doing a sed replacement in the start bash file? https://github.com/mageinferno/docker-magento2-php/blob/77f990d370be9c908ea5f63f3ec432c3ef9f0c42/bin/start#L2

hamdrew commented 8 years ago

@markoshust Yes, that line prevents this bug from affecting the php-fpm service when it is started. However, if you need to run a PHP script within the phpfpm container (eg. bin/magento), PHP uses the value in php.ini.

For example, if you were to remove the -d memory_limit=2G from the mage-setup-raw script, you would run into this bug.

Regardless, this is an invalid value for memory_limit and should either be removed or fixed.

danslo commented 8 years ago

I can't vouch for the fix because I haven't tried it (looks good though), but I do confirm that I have to manually use sed to replace that variable in our build process, since we don't run any of the startup scripts or fpm there (bitbucket pipelines).

markshust commented 8 years ago

@hamdrew thanks for the info, i re-read your initial issue report and it makes sense. let me test this out.

markshust commented 8 years ago

i'm guessing if we apply similar updates to the php.ini and php-fpm configs, we can remove all the php lines from https://github.com/mageinferno/docker-magento2-php/blob/77f990d370be9c908ea5f63f3ec432c3ef9f0c42/bin/start#L2 ?