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

Changes to WORDPRESS_CONFIG_EXTRA ignored unless wp-config.php is deleted #333

Closed
jfahrenkrug opened this Issue Sep 6, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@jfahrenkrug
Copy link

jfahrenkrug commented Sep 6, 2018

First of all, thank you for the hard word on this Docker image!

I have the following issue: I run the Wordpress image with docker-compose and I set several environment variables:

WORDPRESS_DB_HOST: xx
WORDPRESS_DB_USER: root
WORDPRESS_DB_PASSWORD: xx
WORDPRESS_DB_NAME: wordpress

Whenever I change any of those values and re-run docker-compose up, the wp-config.php (which lives on a mounted volume) gets updated.

However, I also have the WORDPRESS_CONFIG_EXTRA environment variable set in my docker-compose.yml. After a morning of trial and error (mostly error) of changing values in WORDPRESS_CONFIG_EXTRA I just realized that those changes were never written to wp-config.php.

I understand that we are using the safe route of not messing with wp-config.php unless it's an easy to find value like WORDPRESS_DB_NAME. It would be nice to provide an option for this, though. It's a bit surprising that changes to all other environment variables are picked up, but not the changes to WORDPRESS_CONFIG_EXTRA.

Could we add marker or delimiter strings around that block? That way we could easily find and replace it. Something like this:

// ---------  WORDPRESS_CONFIG_EXTRA BEGIN ---------
// NOTE: Everything in this block will be overwritten by the WORDPRESS_CONFIG_EXTRA
// environment variable!

<contents of WORDPRESS_CONFIG_EXTRA goes here>

// ---------  WORDPRESS_CONFIG_EXTRA END ---------

Thoughts?

@wglambert wglambert added the question label Sep 6, 2018

@wglambert

This comment has been minimized.

Copy link

wglambert commented Sep 6, 2018

If wp-config.php doesn't exist then add in the WORDPRESS_CONFIG_EXTRA, the other environment variables can be overwritten however.

if [ ! -e wp-config.php ]; then
awk '
/^\/\*.*stop editing.*\*\/$/ && c == 0 {
c = 1
system("cat")
if (ENVIRON["WORDPRESS_CONFIG_EXTRA"]) {
print "// WORDPRESS_CONFIG_EXTRA"
print ENVIRON["WORDPRESS_CONFIG_EXTRA"] "\n"
}
}
{ print }
' wp-config-sample.php > wp-config.php <<'EOPHP'

@tianon

This comment has been minimized.

Copy link
Member

tianon commented Sep 6, 2018

@jfahrenkrug

This comment has been minimized.

Copy link

jfahrenkrug commented Sep 6, 2018

@wglambert Thank you for your reply. I know that that is the current behavior, I found out the hard way this morning :-D I'd like to change that behavior, though :)

@tianon That would be one option, yes. I'd prefer to be able to simply change the value of WORDPRESS_CONFIG_EXTRA in my docker-compose.yml and then have the entrypoint script update the wp-config.php accordingly. IMHO that's the behavior most people would expect.

If we'd look for start and end "markers" in wp-config.php (like I proposed above), it shouldn't be too hard to do, see https://stackoverflow.com/questions/22718708/how-do-i-replace-lines-between-two-patterns-with-a-single-line-in-sed

Am I missing something?

@tianon

This comment has been minimized.

Copy link
Member

tianon commented Sep 7, 2018

My main concern is the fragility aspect -- I don't want to be injecting duplicate configuration if the user edits wp-config.php by hand and accidentally (or intentionally) removes our markers, for example. Similarly, I don't want to be replacing their code if they've gone and modified it (code is a little bit more change-sensitive than the simple define settings we touched before this PR). So given all that, I'd rather take the safe stance and only perform this action when we know it's reasonably safe and reliable for us to do so.

@jfahrenkrug

This comment has been minimized.

Copy link

jfahrenkrug commented Sep 10, 2018

@tianon That makes perfect sense! What do you think about this approach:

  1. Check if the user has supplied the WORDPRESS_CONFIG_EXTRA env variable.
  2. If so, check for the existence of both markers.
  3. If both markers are found, replace the value with WORDPRESS_CONFIG_EXTRA.
  4. If the markers are not found, log some kind of message to make the user aware that the contents of WORDPRESS_CONFIG_EXTRA could not be applied to wp-config.php.

That should be reasonably safe, right? Especially if the markers also contain a warning comment that tells the user not to edit anything between the markers because it will automatically be replaced by WORDPRESS_CONFIG_EXTRA.

@tianon

This comment has been minimized.

Copy link
Member

tianon commented Sep 12, 2018

That's not too bad, but what happens when, for example, we notice a typo in the marker we put in (or want/need to change the wording)? I'm still thinking our best/safest/simplest bet is to let the user know that we couldn't reliably perform the action they requested and leave it at that.

I think folks who really need this sort of behavior are probably better off with something like the following in their WORDPRESS_CONFIG_EXTRA value:

if ($phpCode = getenv('WORDPRESS_CONFIG_EXTRA_EVAL')) {
	eval($phpCode);
}

(As in, directly executing the contents of any WORDPRESS_CONFIG_EXTRA_EVAL environment variable, re-doing the environment variable lookup at each loading of the wp-config.php file.)

@jfahrenkrug

This comment has been minimized.

Copy link

jfahrenkrug commented Sep 12, 2018

@tianon Thank you for that suggestion.
I personally would not like to have an environment variable evaled in my wp-config.php. It would make it difficult to troubleshoot what config options are being used.

I understand your concern about typos and changes to the marker strings. But would that really be an issue in real life? If we agreed on a marker string, would the need really arise to change it in the future? Even if, that could be marked as a breaking change, and the user could be warned if the old marker string is found.

To me the major issue is still that it currently behaves in an unexpected way: All changes to any WORDPRESS_ env are picked up and written to wp-config.php, except for WORDPRESS_CONFIG_EXTRA.

I think if we did these four things, it would be perfectly safe:

  1. Add a starting and ending marker line
  2. Put a large warning comment above the starting marker, warning the user not to edit anything from here till the end of the file.
  3. Replace the contents between the markers if WORDPRESS_CONFIG_EXTRA is set.
  4. Print a warning if WORDPRESS_CONFIG_EXTRA is set but the markers are not found.

This could only fail if the user deliberately ignores the clear directions and warnings we give.

@tianon

This comment has been minimized.

Copy link
Member

tianon commented Sep 20, 2018

But would that really be an issue in real life?

The only reason I bring it up is because it's been an issue we've had before (on other images). 😉

I'm still very wary of adding additional complex behavior to handle this edge case that would amount to exactly the same end-result as the eval I've listed above (the only difference being that it would embed the code directly instead of reading it from an environment variable at runtime, which functionally won't make any difference).

I'm willing to consider a warning or error for users who have set WORDPRESS_CONFIG_EXTRA and it isn't going to be used (perhaps even checking whether the value of the variable can be found in the existing wp-config.php), but any more than that and I'm still somewhere between neutral and opposed leaning towards opposed given both the fragility and the additional maintenance burden in an already somewhat over-complicated entrypoint script.

My own recommendation for folks who need to change their WORDPRESS_CONFIG_EXTRA bits post-install would be to open up the existing wp-config.php file and make changes there. I'd also be happy to accept a documentation PR which makes it more clear that WORDPRESS_CONFIG_EXTRA is an aid for folks who need to add bits to their initial generated wp-config.php, that it only takes effect if we generate that file (ie, others of our provided environment variables are set), and that it will not have any effect if wp-config.php already exists (especially with a pointer to this discussion that has suggestions and workarounds for folks who require an environment variable to adjust the behavior of wp-config.php in arbitrary ways).

hakre added a commit to hakre/wp-clone that referenced this issue Dec 2, 2018

initial commit
development version cloning to local only first.

using git config and resource file to store last project.

.wp-sandbox/index.php file for quick info

using the wordpress docker container package.

and having  docker-library/wordpress#333
which is a pity, but the extra-config.php script for the rescue.

and possbile to inject xdebug on the fly. this costs a bit, should
maybe keep the .so files to spare the compile time each time, WIP.

add some docs, yet missing the manpage.

hakre added a commit to hakre/wp-clone that referenced this issue Dec 2, 2018

initial commit
development version cloning to local only first.

using git config and resource file to store last project.

.wp-sandbox/index.php file for quick info

using the wordpress docker container package.

and having  docker-library/wordpress#333
which is a pity, but the extra-config.php script for the rescue.

and possbile to inject xdebug on the fly. this costs a bit, should
maybe keep the .so files to spare the compile time each time, WIP.

add some docs, yet missing the manpage.

hakre added a commit to hakre/wp-clone that referenced this issue Dec 2, 2018

initial commit
development version cloning to local only first.

using git config and resource file to store last project.

.wp-sandbox/index.php file for quick info

using the wordpress docker container package.

and having  docker-library/wordpress#333
which is a pity, but the extra-config.php script for the rescue.

and possbile to inject xdebug on the fly. this costs a bit, should
maybe keep the .so files to spare the compile time each time, WIP.

add some docs, yet missing the manpage.

hakre added a commit to hakre/wp-clone that referenced this issue Dec 2, 2018

initial commit
development version cloning to local only first.

using git config and resource file to store last project.

.wp-sandbox/index.php file for quick info

using the wordpress docker container package.

and having  docker-library/wordpress#333
which is a pity, but the extra-config.php script for the rescue.

and possbile to inject xdebug on the fly. this costs a bit, should
maybe keep the .so files to spare the compile time each time, WIP.

add some docs, yet missing the manpage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment