Skip to content

Network: display all sites available in network.#6999

Merged
zinigor merged 1 commit intomasterfrom
fix/multisite-display-all-sites
Apr 24, 2017
Merged

Network: display all sites available in network.#6999
zinigor merged 1 commit intomasterfrom
fix/multisite-display-all-sites

Conversation

@jeherve
Copy link
Copy Markdown
Member

@jeherve jeherve commented Apr 19, 2017

By default, get_sites() only displays up to 100 sites by default.

Fixes 170-gh-jpop-issues

Before we ship this, it'd be great to test it on large networks. cc @iandunn

Proposed changelog entry for your changes:

  • Multisite: display all sites available in network, even on large multisite installations.

@jeherve
Network: display all sites available in network.
30b8572 
By default, get_sites() only displays up to 100 sites by default.
@jeherve jeherve added [Focus] Multisite [Pri] Low [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels Apr 19, 2017
@jeherve jeherve self-assigned this Apr 19, 2017
Rodrigoleon
Rodrigoleon reviewed Apr 19, 2017
View reviewed changes
Copy link
Copy Markdown

@Rodrigoleon Rodrigoleon left a comment

Choose a reason for hiding this comment

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

Successfully tested on 360Sites.net which holds 187 sites. Jetpack now displays 186 sites. Main site is missing from the list.

@richardmtl
Copy link
Copy Markdown
Contributor

richardmtl commented Apr 19, 2017

Any way we can force the root site to appear? Note that it also doesn't show up on my multisite on /wp-admin/network/admin.php?page=jetpack

@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented Apr 19, 2017

Any way we can force the root site to appear?

It's hidden on purpose. You can read more about why in #271

@iandunn
Copy link
Copy Markdown

iandunn commented Apr 20, 2017

Tested on wordcamp.org -- 777 sites, not counting the main one -- and it worked as expected :)

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Apr 21, 2017
@jeherve jeherve modified the milestone: 4.9 Apr 24, 2017
@zinigor zinigor merged commit 9535795 into master Apr 24, 2017
@zinigor zinigor deleted the fix/multisite-display-all-sites branch April 24, 2017 18:03
@zinigor zinigor removed the [Status] Ready to Merge Go ahead, you can push that green button! label Apr 24, 2017
jeherve added a commit that referenced this pull request Apr 24, 2017
@jeherve
Changelog: add #6999
73d442c
eliorivero pushed a commit that referenced this pull request Apr 25, 2017
@jeherve @eliorivero
Changelog: 4.9 release. (#6949)
07307cb 
* Changelog: initial commit for 4.9 release.

* Changelog: add #6929

* Changelog: move old changelogs to changelog.txt

* Readme: restore deleted release post link.

The post is now live.

* Changelog: add #6853

* Changelog: add #6856

* Changelog: add #6857

* Changelog: add #6884

* Changelog: add #6885

* Changelog: add #6892

* Changelog: add #6894

* Changelog: add #6898

* Changelog: add #6899

* Changelog: add #6900

* Changelog: add #6909

* Changelog: add #6927

* Changelog: add #6947

* Chagelog: add #6958

* Changelog: add #6961

* Changelog: add #6963

* Changelog: add #6965

* Changelog: add #6986

* Changelog: add #7000

* Changelog: add #7013

* Changelog: add #7015

* Changelog: add #7019

* Changelog: add #7028

* Changelog: add #6998

* Changelog: add #6999

* Changelog: add #7044

* Changelog: add #6881

* Changelog: add #6922

* Changelog: add #6940

* Changelog: add #6962

* Changelog: add #6942

* Changelog: add #6959

* Changelog: add #7018

* Changelog: add #6948

* Changelog: add #6657

* Changelog: add #7030

* Changelog: add #7048

* Changelog: add #7031

* Changelog: add #6990

* Changelog: add #6957

* Changelog: add #7027
@richardmtl
Copy link
Copy Markdown
Contributor

richardmtl commented Apr 26, 2017

A user wrote in about this in 3179447-t and mentioned that they have over 1800 sites. Is the sites list paginated? Will there be any performance issues with this PR, with that amount of sites?

@iandunn
Copy link
Copy Markdown

iandunn commented Apr 26, 2017

Yeah, It's paginated. It used to show all sites before 9a64496, so I don't think this'll have any side-effects; it's just restoring the original behavior.

@iandunn
Copy link
Copy Markdown

iandunn commented Apr 26, 2017

Er, the pagination seems like its implemented in a less-than-ideal way, because it fetches all sites every time, rather than using an OFFSET in the query, but 1800 rows should still just take a fraction of a second for a modern system, especially in a network-admin context where it won't be executed frequently.

If someone has > 100,000 sites, it might start to become an issue, but hopefully we'll React-ify the network admin screens soon and include better ways of dealing with the data, like searching and filtering. I was planning to work on a PR for that, but I've been busier than expected the past few months, so I'm not sure when I'll get to it.

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

Reviewers

1 more reviewer

@Rodrigoleon Rodrigoleon Rodrigoleon left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jeherve jeherve

Labels

Bug When a feature is broken and / or not performing as intended [Focus] Multisite [Pri] Low

Projects

None yet

Milestone

4.9

Development

Successfully merging this pull request may close these issues.

5 participants

@jeherve @richardmtl @iandunn @Rodrigoleon @zinigor