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

Studio: Allow import of wp-config.php #455

Merged
merged 3 commits into from
Aug 13, 2024
Merged

Studio: Allow import of wp-config.php #455

merged 3 commits into from
Aug 13, 2024

Conversation

kozer
Copy link
Contributor

@kozer kozer commented Aug 13, 2024

Fixes https://github.com/Automattic/dotcom-forge/issues/8661

Proposed Changes

When a user has a custom table_prefix, this is not currently respected, as we ignore wp-config.php file.
This PR solves that.

Testing Instructions

  1. Create a new site.
  2. Create a backup from a site with a custom wp_prefix ( You can use the backup located U1WJZT58T/F07GH2T0UAE-slack-file )
  3. Import the backup
  4. Ensure that the correct tables get used after you start your local site.
  5. Check your local site's wp-config.php file, and ensure that you see the correct $table_prefix.

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@kozer kozer requested a review from a team August 13, 2024 09:31
@kozer kozer self-assigned this Aug 13, 2024
Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

I tested it and I confirm it works as expected.
Importing the wp-config file.

src/lib/import-export/import/importers/importer.ts Outdated Show resolved Hide resolved
@@ -65,7 +66,7 @@ describe( 'LocalValidator', () => {
'app/public/wp-admin/wp-admin.php',
'app/public/wp-admin/about.php',
'app/public/wp-includes/test.php',
'app/public/wp-content/wp-config.php',
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@kozer kozer mentioned this pull request Aug 13, 2024
1 task
@kozer kozer merged commit 32b5db9 into trunk Aug 13, 2024
10 checks passed
@kozer kozer deleted the add/wp_config_import branch August 13, 2024 14:03
@jeroenpf
Copy link
Contributor

@kozer I am concerned about the comment by @wojtekn ( p1723457289097709/1723450926.132199-slack-C04GESRBWKW )

If someone is using MySQL instead of SQLite, copying the wp-config file entirely will overwrite the DB credentials.

If we want to prevent that we could read the imported wp-config and update the existing wp-config while excluding the db credentials. You could put the imported wp-config at a temporary location accessible by wp cli and then list the options and update the existing wp-config from that list, skipping the DB options.

Or, we accept that importing on a MySQL Studio site will break the connection. What you think?

@kozer
Copy link
Contributor Author

kozer commented Aug 13, 2024

@jeroenpf , I meanly merged it to resolve https://github.com/Automattic/dotcom-forge/issues/8625#issuecomment-2286095643
I don't know what is the best way to go here. Using MySQL, is not part of Studio currently, so we will fix something for a case we don't support as a tool.
However I see the point.
@wojtekn , what do you think? Do you think it's better to work on a solution like the one Jeroen proposed?
Also, I would love @sejas opinion as well.
If we decide to go with another solution, I'll create an issue and work on it tomorrow, first thing.

@sejas
Copy link
Member

sejas commented Aug 13, 2024

It's a great question. Not only for the the credentials but for other custom code that could live in wp-config.php

I think we should not allow importing sites into MySQL sites or in the other hand we need to create another importer that will execute the correct wp-cli commands and all the other differences.

It's also worth mentioning that users accept a modal to replace all the files and database.

Screenshot 2024-08-13 at 17 14 37

@fluiddot
Copy link
Contributor

I think we should not allow importing sites into MySQL sites or in the other hand we need to create another importer that will execute the correct wp-cli commands and all the other differences.

Yep, I agree. When I was exploring https://github.com/Automattic/dotcom-forge/issues/8606, I also thought we should disable import/export for sites using MySQL (p1723104840771259/1723043486.040399-slack-C04GESRBWKW) as it seems the best way to go until we give support. I can open a PR with the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants