Skip to content

Adjust "wp-config.php" behavior to be skipped if configuration is not provided #206

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
Feb 9, 2017

Conversation

tianon
Copy link
Member

@tianon tianon commented Feb 9, 2017

Also, unset "secret" environment variables before starting Apache/FPM to ensure that a stray phpinfo() doesn't include secrets from our code. (cc @thaJeztah)

Fixes #135

It's worth noting that WordPress now will also detect the absence of wp-config.php, and have a short pre-wizard to configure it, so all configuration can be done directly in the browser (which this PR enables by skipping the wp-config.php behavior if we don't have one of the special WORDPRESS_... vars, or an explicitly linked mysql container sharing environment variables with us).

@tianon
Copy link
Member Author

tianon commented Feb 9, 2017

It doesn't help a ton, but it is easier to review at https://github.com/docker-library/wordpress/pull/206/files?w=1 (ignoring whitespace)

@tianon
Copy link
Member Author

tianon commented Feb 9, 2017

Of course, it fails because wp-config.php is actually more than just a simple configuration file.......

I'll probably just scrape the variables we need directly from the environment instead in that bit of the script (which is still better than taking them in $argv to that php process).

… provided

Also, unset "secret" environment variables before starting Apache/FPM to ensure that a stray `phpinfo()` doesn't include secrets from our code.
@tianon
Copy link
Member Author

tianon commented Feb 9, 2017

Yay 🍏

@yosifkit yosifkit merged commit dd9ecbb into docker-library:master Feb 9, 2017
@yosifkit yosifkit deleted the opt-in branch February 9, 2017 18:49
tianon added a commit to infosiftr/stackbrew that referenced this pull request Feb 9, 2017
- `docker`: 1.13.1
- `golang`: remove `wheezy` for 1.8 (docker-library/golang#144), update `1.8-rc` to be explicit about tracking pre-releases (docker-library/golang#145)
- `irssi`: remove `VOLUME` (jessfraz/irssi#13)
- `postgres`: 9.6.2, 9.3.16, 9.4.11, 9.5.6, 9.2.20, put `docker-entrypoint.sh` explicitly in `PATH` (docker-library/postgres#260)
- `wordpress`: update `wp-config.php` handling to only generate/munge configuration upon user request and to unset all related environment variables explicitly (docker-library/wordpress#206)
@Spittal
Copy link

Spittal commented May 16, 2017

Hey this is still an issue for me. Using the any tagged official wordpress image. But, my preferred tag is 7.1-fpm

Here's the docker-compose.yml

  wordpress:
    image: wordpress
    volumes:
      - ./wp-theme:/var/www/html/wp-content/themes/springboardvr
      - ./wp-config.php:/var/www/html/wp-config.php
    ports:
      - 8081:80
    environment:
      - WORDPRESS_DB_HOST=
      - WORDPRESS_DB_USER=
      - WORDPRESS_DB_PASSWORD=
      - WORDPRESS_DB_NAME=

I fail with the error:
wordpress_1 | sed: cannot rename ./sedGAfbPd: Resource busy

The image works if I comment out the line
- ./wp-config.php:/var/www/html/wp-config.php

@yosifkit
Copy link
Member

It'll only skip setting config values in wp-config.php if you don't supply any of the environment variables, since you can just put them in the config if you are supplying it. The reason for the failing is probably because sed creates a new temporary file to save edits to and then tries to rename it on top of the wp-config.php file you bind-mounted in, which is not possible.

@Spittal
Copy link

Spittal commented May 17, 2017

Oh I see, thanks for the information.

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.

3 participants