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

For non-super-admins, use get_blogs_of_user() to determine authroized sites. #1179

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

peterwilsoncc
Copy link
Collaborator

@peterwilsoncc peterwilsoncc commented Jan 15, 2024

Description of the Change

Modifies the query used to determine a user's authorized sites to use get_blogs_of_user().

As super admins have access to all sites without being a member, the existing get_sites() query is used for super admins.

Closes #1176

How to test the Change

  1. Install Distributor on a Multisite Install in sub-directory mode
  2. Make it big export XDEBUG_MODE=off; for i in {1..1050}; do wp site create --slug=$i --title="MS $i" --url=https://ms-distributor.local; done; (change the URL if needs be)
  3. Create a non super-admin user
  4. Add the new user as an administrator on sites 2, 3 and 1050
  5. Log in/switch to the new user
  6. Visit the pull screen of either site 2 or 3.
  7. The last site should be displayed (1050)

Creating the sites may take some time so you should leave the command going and switch tasks for a while.

Changelog Entry

Fixed - Improved database query for getting a users authorized sites on very large networks.

Credits

Props @jeffpaul, @boonebgorges, @peterwilsoncc.

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@peterwilsoncc peterwilsoncc marked this pull request as ready for review January 16, 2024 01:32
@peterwilsoncc peterwilsoncc requested a review from a team as a code owner January 16, 2024 01:32
@peterwilsoncc peterwilsoncc requested review from Sidsector9 and removed request for a team January 16, 2024 01:32
@jeffpaul jeffpaul added this to the 2.1.0 milestone Jan 16, 2024
@github-actions github-actions bot added the needs:code-review This requires code review. label Apr 2, 2024
@kirtangajjar
Copy link
Member

@peterwilsoncc Tested and ensured that the PR is working for me.

@kirtangajjar
Copy link
Member

@peterwilsoncc It seems that the 1000 site limit problem will still be there for super admins, so why not query for number of sites like this and store it in transients instead of passing a static number:

SELECT COUNT(blog_id) FROM {$wpdb->blogs} WHERE archived = '0' AND spam = '0' AND deleted = '0'

@peterwilsoncc
Copy link
Collaborator Author

Thanks for looking at this @kirtangajjar.

The limit of 1000 sites was added in 3077cf9. Based on the commit message it was a coding standards fix to avoid a warning for an unlimited query.

I'd be surprised if many users were hitting the 1000 site limit but do you think it's worth adding a filter to allow developers to increase the size of the dropdown?

@jeffpaul
Copy link
Member

Fwiw filtering for that seems like a good handling to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Method for assembling list of internal connections does not work well on large installations
4 participants