skilld-labs / skilld-docker-container

Starterkit for drupal development
MIT License
20 stars 32 forks source link

Hardcoded location of extra php settings for sendmail #26

Open andriyun opened 7 years ago

andriyun commented 7 years ago

Here https://github.com/skilld-labs/skilld-docker-container/blob/master/src/docker/docker-compose.override.yml.default#L8 We are adding extra php settings for sendmail

I've faced whit issue when this files don't used whe we using another php version. Reason: another directory for php ini files

andriyun commented 7 years ago

In my case there should be /etc/php7.1/conf.d/90-mail.ini

andypost commented 7 years ago

Confirmed

pabloguerino commented 7 years ago

I suggest that we solve this by changing the .env variables a bit.

We may use only PHP_VERSION and indicate only the version number and nothing else, so you may end up picking one of this:

PHP_VERSION=5.6
PHP_VERSION=7
PHP_VERSION=7.1

then based on PHP_VERSION variable, we can generate:

PHP_IMAGE=$(echo php:$(echo $PHP_VERSION)-fpm)
PHP_CONFIG=$(echo /etc/$(echo $PHP_VERSION))

Note: this is not tested yet, so probably a copy/paste will fail, but we definitely can do this if you guys agree, so will wait for some confirmation and make a PR out of this

pabloguerino commented 7 years ago

Created PR https://github.com/skilld-labs/skilld-docker-container/pull/31


Some things I noted that we can fix,

skilld-docker-container php_vars ▸ docker-compose down
WARNING: The PHP_IMAGE variable is not set. Defaulting to a blank string.
WARNING: The PHP_CONFIG variable is not set. Defaulting to a blank string.

We can add a Makefile task to solve this (make down? make stop?)

Update skilldlabs/docker-php

The naming used for the docker containers is not the optional iif we follow this path, for example for this to work on all versions/containers:

Source: https://github.com/skilld-labs/docker-php/blob/master/Makefile#L2

Or we can avoid to change the names and instead replace the dots inside the PR