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

[#93] Moving the delating to after DB install #98

Merged
merged 2 commits into from
May 30, 2024

Conversation

nathan-schmidt-viget
Copy link
Contributor

Summary

The composer install does not delete the themes or the plugins. These two may to happen after the DB. So trying to run both of these after the DB is set up.

@bd-viget not sure about this fix, but it may be the problem. You may have a better idea on what is going on and why the files are not getting removed.

Issues

Testing Instructions

  1. Can not test till merged with main and we run the install.

Screenshots

Screenshot 2024-05-29 at 3 15 53 PM

Copy link
Contributor

@bd-viget bd-viget left a comment

Choose a reason for hiding this comment

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

Nice catch! This is definitely on me! I believe I changed the code from deleting using PHP (unlink/rmdir all files in the path) to using wp cli, which requires a WP install to do.

That being said, we should probably check for a valid WordPress installation (wp core is-installed) before executing deleteCoreThemes() and deleteCorePlugins() since you can choose to skip the database import step, otherwise you could get the same error if there is no database. - Nothing to worry about ATM, but just making a note.

@nathan-schmidt-viget
Copy link
Contributor Author

@bd-viget I added the check isWordPressInstalled() for both the deleteCoreThemes() and deleteCorePlugins() functions.

@nathan-schmidt-viget nathan-schmidt-viget force-pushed the ns/93-default-theme-plugin branch from 0496e43 to 4e5dcf9 Compare May 30, 2024 15:38
@nathan-schmidt-viget nathan-schmidt-viget merged commit 917f534 into main May 30, 2024
@nathan-schmidt-viget nathan-schmidt-viget deleted the ns/93-default-theme-plugin branch May 30, 2024 15:39
Copy link
Contributor

@bd-viget bd-viget left a comment

Choose a reason for hiding this comment

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

Looks like this didn't post. Sorry!

Comment on lines 93 to 97
// Remove the core Twenty-X themes.
self::deleteCoreThemes();

// Remove Hello Dolly.
self::deleteCorePlugins();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Remove the core Twenty-X themes.
self::deleteCoreThemes();
// Remove Hello Dolly.
self::deleteCorePlugins();
if ( self::isWordPressInstalled() ) {
// Remove the core Twenty-X themes.
self::deleteCoreThemes();
// Remove Hello Dolly.
self::deleteCorePlugins();
}

I was thinking something more along the lines of this. If the user chose to not load a database, there's no reason to report an error, and it only adds the condition in 1 place, rather than 2.

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.

2 participants