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 incorrect handling of admin_email and actual admin user's email when original admin_email user was deleted #603

Merged
merged 2 commits into from
Dec 15, 2022

Conversation

felixarntz
Copy link
Member

Summary

Fixes #601

Relevant technical choices

  • This PR ensures the admin_email option and the admin user's email keep their original values even in case they were different (e.g. when the original admin user was deleted).
  • See SQLite module: Wrong user details #601 (comment) for additional context on the fix.
  • The PR includes one additional (somewhat unrelated) safe guard of not running the automatic setup in a multisite. While it would be great to eventually cover that too, it is a lot more complicated; for now it's the safest option to let the user handle it for a multisite environment.

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@felixarntz felixarntz added [Type] Bug An existing feature is broken Needs Review Anything that requires code review [Focus] Database labels Dec 14, 2022
@felixarntz felixarntz added this to the 1.8.0 milestone Dec 14, 2022
Comment on lines +46 to +48
if ( is_multisite() ) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we also add a check for is_multisite in the can-load.php file? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

If we add the is_multisite check in the can-load.php file, we don't need to add it to the activate.php file. WDYT?

Choose a reason for hiding this comment

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

Yes, I think this is better candidate to go in can-load.php as if site is multisite, then we should instruct not to load module at all rather than instructing to load module and then simply doing nothing!

Copy link
Member Author

Choose a reason for hiding this comment

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

@aristath @mukeshpanchal27 @ankitrox This shouldn't be necessary. It's not that the entire module doesn't work on multisite, it's just that the automatic installation doesn't work with multisite.

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks @felixarntz The PR resolved the user email issue, but it still did not pull user details such as first name, last name, website, biographical information, and so on.

Can we fix this edge case issue in the next release? WDYT?

@mukeshpanchal27 mukeshpanchal27 added [Module] SQLite and removed Needs Review Anything that requires code review labels Dec 15, 2022
@ankitrox
Copy link

Apart from this comment, everything else looks fine to me.

Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

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

I'll go ahead and pre-approve this. Moving the is_multisite check to can-load is not a deal-breaker, I'm happy to merge this one as-is if we don't want to move the multisite check. cc @felixarntz

@felixarntz
Copy link
Member Author

@mukeshpanchal27

The PR resolved the user email issue, but it still did not pull user details such as first name, last name, website, biographical information, and so on.

I think that's expected. We explicitly say that this setup is not a migration of data, we only set up the bare minimum WordPress SQLite database based on the data from the original database. User details like those you're saying are not essential to a WordPress install (i.e. they are not encompassed by wp_install()).

@felixarntz felixarntz merged commit 3234632 into trunk Dec 15, 2022
@felixarntz felixarntz deleted the fix/601-incorrect-sqlite-admin-user-email branch December 15, 2022 16:14
@dianatupas
Copy link

dianatupas commented Feb 17, 2023

This bug is still existing

@eclarke1 eclarke1 moved this from Review to Done in [Focus] Database Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature is broken
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

SQLite module: Wrong user details
5 participants