Skip to content
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

Use WordPress setup default values for env variables #577

Merged
merged 2 commits into from
Mar 17, 2021

Conversation

yosifkit
Copy link
Member

@yosifkit yosifkit commented Mar 12, 2021

This brings back the default values from before #572 and #557

Edit: Use WordPress setup defaults; also allow empty string values.

@tianon
Copy link
Member

tianon commented Mar 12, 2021

I'm actually -1 on this change -- encouraging/enabling users to use the MySQL root user or an empty password was a mistake IMO (for the same reasons as having this container opportunistically create the database -- the database user given really shouldn't have that level of privileged access 😬), and minor positive breaking changes like this are why we had this newer functionality available for users to pre-test for several months before we enabled it (and even waited for a new release of WordPress itself to do so). 😕

Additionally, for this file to even be used they'd have to be setting some WORDPRESS_... variables (otherwise we don't copy it so that the interactive wizard still works), and IMO asking them to set a few more explicitly isn't unreasonable.

I suppose one improvement we arguably should make is to update our getenv wrapper to differentiate unset environment variables from empty such that the (discouraged!!) configuration is at least possible? (#574)

@yosifkit
Copy link
Member Author

That sounds reasonable; should we at least restore WORDPRESS_DB_NAME to default to wordpress? If we keep the new default, then someone is going to assume that they can rely on it and set just MYSQL_DATABASE=database_name_here on MySQL. 😬

@tianon
Copy link
Member

tianon commented Mar 12, 2021

Hmmmm, good point -- I took these values from wp-config-sample.php, but I'm not sure whether there's much value in that (since the whole point of this is that users can use environment variables instead and thus don't need to edit wp-config.php manually).

Perhaps we should look to the built-in setup wizard and match any default values it pre-fills/suggests?

@tianon
Copy link
Member

tianon commented Mar 12, 2021

https://github.com/WordPress/WordPress/blob/fb8c18e54198ad9364213d792ea39b2008d36725/wp-admin/setup-config.php#L218 corroborates wordpress as a default value 👍

(I don't know whether example username and example password as defaults are really any better or worse or indifferent from password_here etc 😄)

@yosifkit
Copy link
Member Author

(I don't know whether example username and example password as defaults are really any better or worse or indifferent from password_here etc )

If we move to whatever upstream uses as defaults, then we can just point to that in the future (minus localhost for database host, because containers make localhost database very unlikely).

@yosifkit yosifkit changed the title Restore previous default values for env variables Use WordPress setup default values for env variables Mar 15, 2021
@yosifkit
Copy link
Member Author

Just tested this stack.yml to work on this new change:

version: '3.1'

services:

  wordpress:
    image: wordpress:test
    restart: always
    ports:
      - 8080:80
    environment:
      WORDPRESS_DB_HOST: db
      WORDPRESS_DB_USER: root
      WORDPRESS_DB_PASSWORD: ''
      WORDPRESS_DB_NAME: exampledb
    volumes:
      - wordpress:/var/www/html

  db:
    image: mysql:5.7
    restart: always
    environment:
      MYSQL_DATABASE: exampledb
      MYSQL_ALLOW_EMPTY_PASSWORD: '1'
    volumes:
      - db:/var/lib/mysql

volumes:
  wordpress:
  db:


/** MySQL database password */
define( 'DB_PASSWORD', getenv_docker('WORDPRESS_DB_PASSWORD', 'password_here') );
define( 'DB_PASSWORD', getenv_docker('WORDPRESS_DB_PASSWORD', 'example password') );

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think there's a good place in here somewhere we could inject a link to https://github.com/WordPress/WordPress/blob/f9cc35ebad82753e9c86de322ea5c76a9001c7e2/wp-admin/setup-config.php#L216-L230 to show/remind where these defaults came from? Perhaps something generic here at the end like this?

Suggested change
// Docker image fallback values above are sourced from the official WordPress installation wizard:
// https://github.com/WordPress/WordPress/blob/f9cc35ebad82753e9c86de322ea5c76a9001c7e2/wp-admin/setup-config.php#L216-L230
// (However, using "example username" and "example password" in your database is strongly discouraged. Please use strong, random credentials!)

@yosifkit
Copy link
Member Author

Added fix for removing newlines from _FILE contents: #576 (comment)

@tianon tianon merged commit 0f81a03 into docker-library:master Mar 17, 2021
@tianon tianon deleted the restore-default-env-values branch March 17, 2021 16:22
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request Mar 17, 2021
Changes:

- docker-library/wordpress@0f81a03: Merge pull request docker-library/wordpress#577 from infosiftr/restore-default-env-values
- docker-library/wordpress@c313a26: Remove newlines from `_FILE` contents
- docker-library/wordpress@95ca33a: Use WordPress setup default values for env variables; use empty env values
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

Successfully merging this pull request may close these issues.

2 participants