We could also add a default to this when env variable doesn't exist: ${DOCKER_PHP_VERSION:-7.2-apache}
— thus not needing .env file in the root folder?

We do have a default in the Dockerfile here:

https://github.com/Automattic/jetpack/pull/9252/files#diff-ebacf6f6ae4ee68078bb16454b23247dR1

Now that I think about it, it doesn't seem like we need the .env file at all for this thing given that the yarn docker:up:version commands will be setting the env var

Now that I think about it, it doesn't seem like we need the .env file at all for this thing given that the yarn docker:up:version commands will be setting the env var

If you take out the .env and don't have a default defined in docker-compose.yml, you'll see a warning:

WARNING: The DOCKER_PHP_VERSION variable is not set. Defaulting to a blank string.

Things would still work but defining a default would take care of the warning:

    build:
      context: .
      args:
        PHP_VERSION: ${DOCKER_PHP_VERSION:-7.0-apache}
    image: jetpack_wordpress:${DOCKER_PHP_VERSION:-7.0-apache}

I wonder what happens to other docker-compose commands if PHP version change isn't persistent in .env file.
If after initial yarn docker:up:version you want to run any other command, wouldn't they pick the default PHP version and re-build the image?

yeah you're right. I think I didn't prepend an evn to the regular yarn docker:up either. I like your solution

Adding to this that e.g. yarn docker:clean script failed for me after playing with versions:

ERROR: Failed to remove image for service wordpress: 404 Client Error: Not Found ("No such image: jetpack_wordpress:7.0")

We'll have to install different versions of phpunit depending on php-version:

Specifically phpunit-5.7.5 won't work with php-7.2.

Not sure if our tests support phpunit6 yet, though. I remember fiddling with them a bit to get them work some months ago.

via https://phpunit.de

What do you think if we do this here instead:

ARG PHP_VERSION=7.0
FROM php:${PHP_VERSION}-apache

...because we're not really supporting any other types of images but -apache so manually typing it in everywhere doesn't make sense?

If this would just be a version number, it would be compatible with existing configs and commands (i.e. DOCKER_PHP_VERSION=5 yarn docker:up) if we ever decide to change the way PHP gets installed.

What do you think if we drop DOCKER_ out from this?

I'd love to be able to type PHP_VERSION=5 yarn docker:up where 5 could be any version we didn't include in yarn scripts.

Noting here also that variables modifying Docker/Docker-compose CLI all start with DOCKER_ so we shouldn't create names that can be confused with those.

Shall we add v5 here? https://hub.docker.com/_/php/

I was exploring if it would be possible to do yarn docker:up --php=7.1 or something similar but I didn't really find a way to grab those args in a way that would work windows/linux/mac and wouldn't require adding extra node/bash scripts.

Yeah we should. It's just that some build step was throwing an error for me with 5.6 and I was lazy to solve it at the moment.

Just wanted to get the flow of multi php working with this image and prove if it sucked or not to switch versions in this way. Not still quite sure if it's comfortable or not. It did come in handy to have it since I pushed this changes.

Yeah, developer experience definitely is a big part of this so I wouldn't rush this in just when it technically already works.

indeed, neither would I.

I almost feel like having ENTRYPOINT echo "PHP: $PHP_VERSION" or something along those lines would make it clear what version of PHP I'm running each time.