-
Notifications
You must be signed in to change notification settings - Fork 31
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
Adding network support #582
Conversation
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.
Great work so far!
wp-parsely.php
Outdated
require __DIR__ . '/src/UI/class-settings-page-network.php'; | ||
require __DIR__ . '/src/UI/class-parsely-sites-table.php'; | ||
add_action( | ||
'_network_admin_menu', |
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.
Is there a good reason to hook into _network_admin_menu
instead of network_admin_menu
?
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.
It would collide with the action in class-settings-page-network.php
. I've added a change that gets rid of it and plays with priorities, but I'm not sure about it.
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.
Ah. I see. _network_admin_menu
works for me, then!
src/UI/class-parsely-sites-table.php
Outdated
*/ | ||
private function fetch_table_data(): array { | ||
$parsely_network_sites = array(); | ||
foreach ( get_sites() as $site ) { |
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 think get_sites
will cap at 100 results per:
https://github.com/WordPress/wordpress-develop/blob/5.8.1/src/wp-includes/class-wp-site-query.php#L176
So, we should probably either paginate or add an indication somewhere that only the first 100 results will be shown. I think the latter is fine. If someone is running that many subsites, they're likely going to be reaching for a scripting approach to management.
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 see number is a parameter, so we could hypothetically bump that number up. I would expect a performance drop if that number is very big, though.
I like your messaging proposal. We can implement pagination later if we see a lot of demand for it.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Closing in favor of #583 |
Description
Closes #575
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate)