Skip to content

[Symfony CLI] Docker integration and MailCatcher prefix #15424

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

Merged
merged 1 commit into from
Aug 10, 2021

Conversation

Kocal
Copy link
Member

@Kocal Kocal commented Jun 8, 2021

Hi 👋

Given my docker-compose.yaml:

version: '3.6'

volumes:
    db-data:
    redis-data:

services:

  database:
    image: 'mysql:5.7'
    ports: [3306]
    environment:
      MYSQL_USER: 'app'
      MYSQL_PASSWORD: 'app'
      MYSQL_DATABASE: 'app'
      MYSQL_ALLOW_EMPTY_PASSWORD: 'yes'
      TZ: Etc/UTC
    volumes:
      - db-data:/var/lib/mysql
      - .manala/init-db:/docker-entrypoint-initdb.d
    healthcheck:
      test: mysqladmin ping --silent
      interval: 10s
      timeout: 5s
      retries: 5

  phpmyadmin:
    image: phpmyadmin
    ports: [80]
    environment:
      PMA_HOST: 'database'
      PMA_USER: 'app'
      PMA_PASSWORD: 'app'

  redis:
    image: 'redis:alpine'
    ports: [6379]
    environment:
      TZ: Etc/UTC
    volumes:
      - redis-data:/data

  phpredisadmin:
    image: erikdubbelboer/phpredisadmin
    ports: [80]
    environment:
      REDIS_1_HOST: 'redis'

  mailcatcher:
    image: 'schickling/mailcatcher'
    ports: [1025, 1080]
    environment:
      TZ: Etc/UTC

When running docker-compose up and symfony var:export --multiline, the env vars for MailCatcher are prefixed (by default) by MAILCATCHER_ but not by MAILER_:
image

Hi 👋 

Given my `docker-compose.yaml`: 

```yaml
version: '3.6'

volumes:
    db-data:
    redis-data:

services:

  database:
    image: 'mysql:5.7'
    ports: [3306]
    environment:
      MYSQL_USER: 'app'
      MYSQL_PASSWORD: 'app'
      MYSQL_DATABASE: 'app'
      MYSQL_ALLOW_EMPTY_PASSWORD: 'yes'
      TZ: Etc/UTC
    volumes:
      - db-data:/var/lib/mysql
      - .manala/init-db:/docker-entrypoint-initdb.d
    healthcheck:
      test: mysqladmin ping --silent
      interval: 10s
      timeout: 5s
      retries: 5

  phpmyadmin:
    image: phpmyadmin
    ports: [80]
    environment:
      PMA_HOST: 'database'
      PMA_USER: 'app'
      PMA_PASSWORD: 'app'

  redis:
    image: 'redis:alpine'
    ports: [6379]
    environment:
      TZ: Etc/UTC
    volumes:
      - redis-data:/data

  phpredisadmin:
    image: erikdubbelboer/phpredisadmin
    ports: [80]
    environment:
      REDIS_1_HOST: 'redis'

  mailcatcher:
    image: 'schickling/mailcatcher'
    ports: [1025, 1080]
    environment:
      TZ: Etc/UTC
```

When running `docker-compose up` and `symfony var:export --multiline`, the env vars for MailCatcher are prefixed (by default) by `MAILCATCHER_` but not by `MAILER_`.
@carsonbot carsonbot added this to the 4.4 milestone Jun 8, 2021
@OskarStark
Copy link
Contributor

If you rename your service to Kocal the will be prefixed with KOCAL_

@Kocal
Copy link
Member Author

Kocal commented Jun 8, 2021

Ah you're right, I've totally forgot about that...

However this list looks wrong to me:
image

What's the point to document the Symfony default prefix given the configured port, if in fact it use the container's name for the prefix? 😕

@wouterj
Copy link
Member

wouterj commented Jun 17, 2021

@fabpot can you maybe help here?

@javiereguiluz
Copy link
Member

I'm merging this because in the changelog of v4.16.0 (from June 8, 2020) we can see this:

[BC BREAK] Fix Mailer exposed env vars in a local Docker Compose environment The Docker mailcatcher service env var names (might have) changed depending on your Docker Compose configuration. Instead of always using MAILER_ and MAIL_ prefixes for the SMTP service, we are now using the service name and service prefix like any other services defined in .docker-compose.yml.

@javiereguiluz
Copy link
Member

Thank you Hugo.

@javiereguiluz javiereguiluz merged commit af0f479 into symfony:4.4 Aug 10, 2021
@Kocal Kocal deleted the patch-1 branch August 10, 2021 20:08
@Kocal
Copy link
Member Author

Kocal commented Aug 10, 2021

Oh, so this was a special case for Mailer/Mailcatcher... Thanks for the investigation Javier!

@fabpot
Copy link
Member

fabpot commented Aug 11, 2021

This must be reverted. The docs were right. There is nothing special with mailer. As discussed above, it depends on the name of the service. And by default, people should use mailer as a service name as Symfony expects MAILER_* env vars.

@fabpot
Copy link
Member

fabpot commented Aug 11, 2021

The second column is the Symfony expected env var prefix.

@Kocal
Copy link
Member Author

Kocal commented Aug 11, 2021

So it's the wording Symfony default prefix which is wrong?

@fabpot
Copy link
Member

fabpot commented Aug 11, 2021

No, it’s correct

javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Aug 11, 2021
@Kocal
Copy link
Member Author

Kocal commented Aug 11, 2021

But what this column provides as information?

If env vars are prefixed by the docker service name (which is the case), we don't need to know what Symfony expects as prefix for env vars, right, since the user decides which env vars to inject to Symfony?

Or am I missing something?

EDIT: If in my docker-compose.yaml I have the following service:

  foobar:
    image: 'schickling/mailcatcher'
    ports: [1025, 1080]
    environment:
      TZ: Etc/UTC

This is what symfony var:export --multiline outputs:

export FOOBAR_AUTH_MODE=
export FOOBAR_DRIVER=smtp
export FOOBAR_DSN=smtp://127.0.0.1:32797
export FOOBAR_HOST=127.0.0.1
export FOOBAR_PASSWORD=
export FOOBAR_PORT=32797
export FOOBAR_URL=smtp://127.0.0.1:32797
export FOOBAR_USERNAME=
export FOOBAR_WEB_HOST=127.0.0.1
export FOOBAR_WEB_IP=127.0.0.1
export FOOBAR_WEB_PORT=32796
export FOOBAR_WEB_SCHEME=http
export FOOBAR_WEB_SERVER=http://127.0.0.1:32796
export FOOBAR_WEB_URL=http://127.0.0.1:32796
export MAILER_CATCHER=1

And I can use the following code in config/packages/mailer.yaml:

framework:
    mailer:
        dsn: '%env(FOOBAR_DSN)%'

So what's the point of this column? 😕

javiereguiluz added a commit that referenced this pull request Aug 11, 2021
This PR was merged into the 4.4 branch.

Discussion
----------

Revert the changes made in #15424

Reverts the changes made in #15424 because of #15424 (comment)

Commits
-------

b61d2a9 Revert the changes made in #15424
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Aug 11, 2021
* 4.4:
  Revert the changes made in symfony#15424
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Aug 11, 2021
* 5.3:
  Revert the changes made in symfony#15424
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Aug 11, 2021
* 5.4:
  Revert the changes made in symfony#15424
@wouterj
Copy link
Member

wouterj commented Aug 11, 2021

It depends on what you think "Symfony" means in "Symfony default prefix", and I must admit that I also wrongly understood this up until now. It means "The Symfony Framework recipes expect these prefixes to be used by service", it doesn't mean "The Symfony CLI uses this prefix".

E.g. if you look at the Symfony Flex mailer recipe, you'll see that MAILER_URL is used. So what this column is actually saying: "If you want to use these automatic vars without changing the config, make sure you name the MailCatcher service mailer"

@Kocal
Copy link
Member Author

Kocal commented Aug 11, 2021

Alright, so to me there is definitely a wording issue. 😕

What about:

  • Renaming Symfony default prefix to Expected Symfony env var prefix
  • and adding a column Recommended Docker service name

image

?

@wouterj
Copy link
Member

wouterj commented Aug 11, 2021

I agree.

I think we should drop the prefix column completely and only have the "Recommended Docker service name" column. Then, explain above/below this table that Symfony CLI uses the Docker container name as prefix (e.g. mailer becomes MAILER_).

@fabpot
Copy link
Member

fabpot commented Aug 19, 2021

Re-reading the text above, I think everything is already well explained.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants