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

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

Merged
merged 9 commits into from
Mar 12, 2021

Conversation

ribaricplusplus
Copy link
Member

Description

This is an addition to #29770

#29770 Seems to fix the access denied error, but then a new error arises described in this comment #29770 (comment)

This PR fixes that error and it should close issue #29752

How has this been tested?

Running wp-env start from an empty directory

@ribaricplusplus ribaricplusplus changed the title Fix/wp env docker compose Fix Error: Could not process the 'wp-config.php' transformation Mar 11, 2021
Comment on lines 143 to 163
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

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.

@ribaricplusplus ribaricplusplus marked this pull request as ready for review March 11, 2021 20:43
Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

Note: I have not run wp-env in several weeks, so my docker image probably wasn't updated at first.

When running npx wp-env start, I first got this issue:

Screen Shot 2021-03-11 at 2 01 48 PM

After running npx wp-env start --update, I get the following error:

~/source/gutenberg(fix/wp-env-docker-compose) » npx wp-env start --update                                                                                            noahtallen@Noah-MBP16
✖ Error while running docker-compose command.
Creating a86349ed83984e487ba92af16fae23af_cli_run ... done
mysqlcheck: Got error: 1045: Access denied for user 'root'@'172.18.0.6' (using password: YES) when trying to connect

@ribaricplusplus
Copy link
Member Author

You get that error because the database wordpress that root is trying to connect to doesn't exist. And it doesn't exist because MariaDB image doesn't create a new database as specified in the environment variables if a volume is already mounted.

So if you delete volumes with wp-env destroy and then do wp-env start again, it's going to work.

I don't know if there's anything we can do about this.

@noahtallen
Copy link
Member

Is this just a one-time problem?

@ribaricplusplus
Copy link
Member Author

Because of changes by @youknowriad from #29770 (which this PR extends), the database is always going to be created properly in the future. So it should be a one time problem.

@noahtallen
Copy link
Member

Login seems to be broken. using root instead of username doesn't work either:

Screen Shot 2021-03-11 at 2 17 04 PM

It successfully starts, though!

Note that the PHPunit tests appear to be broken. Seems like we might have to debug the bootstrap file to get more helpful information

@ribaricplusplus
Copy link
Member Author

ribaricplusplus commented Mar 11, 2021

Username: admin
Password: password
Works for me. Those don't work for you?

@noahtallen
Copy link
Member

Ah, FML, not sure why that slipped my mind! Thanks :)

Looking into the PHPunit stuff now

@noahtallen
Copy link
Member

Hm. npm run test-unit-php passes locally.

@tmdk
Copy link
Contributor

tmdk commented Mar 11, 2021

The phpunit container is still missing the WORDPRESS_DB_* environment variables. Tests were failing for me because of this with "wpdie called" as the only output.

@noahtallen
Copy link
Member

Interesting, I wonder why they passed locally for me in that case. It totally makes sense though.

@ribaricplusplus it might make sense to pull out the wordpress constants into their own object so that we don't have to repeat them everywhere. e.g.:

const databaseEnvironment = {
	WORDPRESS_DB_NAME: 'tests-wordpress',
	WORDPRESS_DB_USER: 'root',
	WORDPRESS_DB_PASSWORD: 'password',
	WORDPRESS_DB_HOST: 'tests-mysql',
};

// later on, in the compose config:
environment: {
  ...databaseEnvironment,
}

},
volumes: [ 'mysql:/var/lib/mysql' ],
},
'tests-mysql': {
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

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.

},
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.

@ribaricplusplus
Copy link
Member Author

@noahtallen Did that with the last commit.

Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

Alright, let's merge this now to get everything working. For follow up:

  • Figure out a better way to consume Docker (perhaps our own docker image, or one of the ones maintained by wordpress) so that we don't run into this issue again.
  • Move away from a separate tests-mysql, perhaps creating the test DB manually in the installer script.
  • Resolve anything else that might not be working (unsure if anything else is broken.)
  • Push this change to npm ASAP.

I'm stepping away soon, so passing the torch on to the next person :)

@noahtallen noahtallen merged commit f8052e1 into WordPress:trunk Mar 12, 2021
@github-actions github-actions bot added this to the Gutenberg 10.3 milestone Mar 12, 2021
@gziolo gziolo modified the milestones: Gutenberg 10.3, Gutenberg 10.2 Mar 17, 2021
gziolo pushed a commit that referenced this pull request Mar 17, 2021
- Use separate tests database service
- Configure database credentials in every service
- Set wp-config.php to be writable

Co-authored-by: Riad Benguella <[email protected]>
Co-authored-by: Noah Allen <[email protected]>
@gziolo
Copy link
Member

gziolo commented Mar 17, 2021

I cherry-picked commit with this PR to release/10.2 branch.

@talldan talldan added [Tool] Env /packages/env [Type] Bug An existing feature does not function as intended labels Mar 22, 2021
noisysocks pushed a commit that referenced this pull request Apr 6, 2021
- Use separate tests database service
- Configure database credentials in every service
- Set wp-config.php to be writable

Co-authored-by: Riad Benguella <[email protected]>
Co-authored-by: Noah Allen <[email protected]>
noisysocks pushed a commit that referenced this pull request Apr 6, 2021
- Use separate tests database service
- Configure database credentials in every service
- Set wp-config.php to be writable

Co-authored-by: Riad Benguella <[email protected]>
Co-authored-by: Noah Allen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] Env /packages/env [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants