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.

Sign up for GitHub

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

Fix Error: Could not process the 'wp-config.php' transformation #29800

Merged
merged 9 commits into from
Mar 12, 2021
Merged

Fix Error: Could not process the 'wp-config.php' transformation #29800

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2742537
Fix wp-env docker-compose config with explicit users and passwords
youknowriad Mar 11, 2021
7d660da
try restoring mysql volumes
youknowriad Mar 11, 2021
1f5efe7
Fix wp-config.php is not writable error
ribaricplusplus Mar 11, 2021
42058f4
Cleaner fix implementation
ribaricplusplus Mar 11, 2021
3f2c8af
Export new function
ribaricplusplus Mar 11, 2021
a982f82
Abstract db environment into constants
ribaricplusplus Mar 11, 2021
d62f7b8
Revert "Abstract db environment into constants"
ribaricplusplus Mar 12, 2021
0ca4422
Abstract db environment into constants
ribaricplusplus Mar 11, 2021
9a7f8c3
Add db credentials to phpunit image
noahtallen Mar 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions packages/env/lib/build-docker-compose-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,26 +176,41 @@ module.exports = function buildDockerComposeConfig( config ) {
image: 'mariadb',
ports: [ '3306' ],
environment: {
MYSQL_ALLOW_EMPTY_PASSWORD: 'yes',
MYSQL_ROOT_PASSWORD: 'password',
MYSQL_DATABASE: 'wordpress',
},
volumes: [ 'mysql:/var/lib/mysql' ],
},
'tests-mysql': {
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of adding an extra MySQL container. It will mean there's a lot of extra memory usage and startup time.

Copy link
Member

Choose a reason for hiding this comment

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

How about we keep to a single container and create two databases ourselves using wp db create?

https://developer.wordpress.org/cli/commands/db/create/

Copy link
Member

Choose a reason for hiding this comment

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

Could that be a follow-up task? It seems higher effort than the current approach

Copy link
Member

Choose a reason for hiding this comment

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

Yep absolutely.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about using one database, and different table prefixes for dev and tests, respectively? The wordpress docker image supports a WORDPRESS_TABLE_PREFIX env variable for that purpose.

image: 'mariadb',
ports: [ '3306' ],
Copy link
Member

Choose a reason for hiding this comment

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

This port is already being used by the mysql container above.

Copy link
Member Author

@ribaricplusplus ribaricplusplus Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think those ports just denote which ports are exposed within a container. Ports are isolated within individual containers, so if I'm not mistaken there should be no conflict.

EDIT: According to compose specification this short notation means that the port 3306 inside the container is going to be assigned an ephemeral port on the host machine. Still, that means there should be no trouble.

environment: {
MYSQL_ROOT_PASSWORD: 'password',
MYSQL_DATABASE: 'tests-wordpress',
},
volumes: [ 'mysql-test:/var/lib/mysql' ],
},
wordpress: {
build: '.',
depends_on: [ 'mysql' ],
image: developmentWpImage,
ports: [ developmentPorts ],
environment: {
WORDPRESS_DB_NAME: 'wordpress',
WORDPRESS_DB_USER: 'root',
WORDPRESS_DB_PASSWORD: 'password',
},
volumes: developmentMounts,
},
'tests-wordpress': {
depends_on: [ 'mysql' ],
depends_on: [ 'tests-mysql' ],
image: testsWpImage,
ports: [ testsPorts ],
environment: {
WORDPRESS_DB_NAME: 'tests-wordpress',
WORDPRESS_DB_USER: 'root',
WORDPRESS_DB_PASSWORD: 'password',
WORDPRESS_DB_HOST: 'tests-mysql',
},
volumes: testsMounts,
},
Expand All @@ -204,12 +219,23 @@ module.exports = function buildDockerComposeConfig( config ) {
image: developmentWpCliImage,
volumes: developmentMounts,
user: cliUser,
environment: {
WORDPRESS_DB_NAME: 'wordpress',
WORDPRESS_DB_USER: 'root',
WORDPRESS_DB_PASSWORD: 'password',
},
},
'tests-cli': {
depends_on: [ 'tests-wordpress' ],
image: testsWpCliImage,
volumes: testsMounts,
user: cliUser,
environment: {
WORDPRESS_DB_NAME: 'tests-wordpress',
WORDPRESS_DB_USER: 'root',
WORDPRESS_DB_PASSWORD: 'password',
WORDPRESS_DB_HOST: 'tests-mysql',
},
},
composer: {
image: 'composer',
noahtallen marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -235,6 +261,7 @@ module.exports = function buildDockerComposeConfig( config ) {
...( ! config.coreSource && { wordpress: {} } ),
...( ! config.coreSource && { 'tests-wordpress': {} } ),
mysql: {},
'mysql-test': {},
'phpunit-uploads': {},
},
};
Expand Down
21 changes: 21 additions & 0 deletions packages/env/lib/commands/start.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,27 @@ module.exports = async function start( { spinner, debug, update, xdebug } ) {
: [],
} );

await dockerCompose.exec( 'wordpress', 'chown www-data:www-data wp-config.php', {
config: dockerComposeConfigPath,
log: debug,
} )

await dockerCompose.exec( 'wordpress', 'chmod 777 wp-config.php', {
config: dockerComposeConfigPath,
log: debug,
} )

await dockerCompose.exec( 'tests-wordpress', 'chown www-data:www-data wp-config.php', {
config: dockerComposeConfigPath,
log: debug,
} )

await dockerCompose.exec( 'tests-wordpress', 'chmod 777 wp-config.php', {
config: dockerComposeConfigPath,
log: debug,
} )


Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that manually changing permissions for wp-config.php is the right fix, see discussion at #29770 😅

Copy link
Member Author

@ribaricplusplus ribaricplusplus Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think you have a good point that configuration should be set idiomatically via docker-compose configuration.

However, I think it would still make sense to fix wp-config.php permissions, because currently wp-config.php is owned by root and so any commands that need to use the config for any reason would fail.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

We've had to update permissions for some of these files to get them to work in wp-env before, so I'm not against it

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should use WORDPRESS_CONFIG_EXTRA because the wordpress image will only use that variable if wp-config.php doesn't already exist which isn't very flexible for developers. For example: you wouldn't be able to update WP_DEBUG in .wp-env.json, restart wp-env, and see WordPress running in debug mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should use WORDPRESS_CONFIG_EXTRA because the wordpress image will only use that variable if wp-config.php doesn't already exist

I'm not totally sure, but I think that's no longer true per docker-library/wordpress#557, which has wp-config.php read those values dynamically from the docker env (using a getenv() helper); see e.g.

// Only run WordPress install/configuration when config has changed.
if ( shouldConfigureWp ) {
spinner.text = 'Configuring WordPress.';
Expand Down