-
Notifications
You must be signed in to change notification settings - Fork 107
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
Conversation
…original admin_email user was deleted.
if ( is_multisite() ) { | ||
return; | ||
} |
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this 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?
Apart from this comment, everything else looks fine to me. |
There was a problem hiding this 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
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 |
This bug is still existing |
Summary
Fixes #601
Relevant technical choices
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).Checklist
[Focus]
orInfrastructure
label.[Type]
label.no milestone
label.