Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#41146 closed enhancement (fixed)

Add filter for a site's class (WP_MS_Users_List_Table)

Reported by: kraftbj's profile kraftbj Owned by: desrosj's profile desrosj
Milestone: 5.2 Priority: normal
Severity: normal Version:
Component: Networks and Sites Keywords: has-patch
Focuses: administration Cc:

Description

In the WP_MS_Users_List_Table, each site that is listed is span'd with a class of site-# where # is the ID of the network for a site.

Adding a filter to this class would allow site administrators to add addition classes to this output which would easily allow for styling of site items based on the additional classes.

Attachments (4)

41146.diff (1.1 KB) - added by kraftbj 7 years ago.
Adds filter to the span class
41146.2.diff (1.2 KB) - added by kraftbj 7 years ago.
converted to array and other suggested edits
41146.3.diff (1.3 KB) - added by kraftbj 7 years ago.
updated docs and other suggested changes
41146.4.diff (1.5 KB) - added by kraftbj 6 years ago.
Addressed JJJ's comments.

Download all attachments as: .zip

Change History (19)

@kraftbj
7 years ago

Adds filter to the span class

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


7 years ago

#2 @desrosj
7 years ago

  • Keywords needs-patch added; has-patch removed

Thanks for the initial patch @kraftbj. This sounds like a good addition. Some thoughts:

  • I think that this filter should pass an array of classes instead of a string. This will help prevent situations where someone adds a class but forgets to add a space, causing classes to bleed together.
  • The filter should be on its own line to make it more readable.
  • An array_unique() call would also help prevent duplicate classes.
  • Now that there is a filter, whatever value is output into the class attribute will need to be passed through esc_attr().

@kraftbj
7 years ago

converted to array and other suggested edits

#3 @kraftbj
7 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to kraftbj
  • Status changed from new to accepted

Made the suggested changes. I'm only checking for is_array to make sure the return is in the right format. Someone can return false, etc to short-circuit completely.

We could also check for an empty array, but I don't want to overengineer it.

#4 @flixos90
7 years ago

@kraftbj

I'm only checking for is_array to make sure the return is in the right format.

I think we can even strip this out, as it's commonly expected to return the same type that is passed to a filter as the filtered parameter. Similar filters in core do not check this either.

We could also check for an empty array, but I don't want to overengineer it.

Right, I don't think that's necessary, especially since the default isn't empty either.

There are just a few (mostly naming) changes I'd like to suggest:

  • The variable could be called $site_classes to indicate that we're dealing with multiple classes (also makes it more obvious that it's an array).
  • While the site ID property is called $userblog_id (for legacy reasons), let's use a more appropriate name in the filter documentation (for example $site_id). The name is independent of the actual passed parameter anyway since it's a property.
  • It might be useful to pass the network ID (as saved in the $site_id property) to the filter as well, since that is also used in the default class. Similar to the above, the docs should probably call it $network_id.
  • Not very important, but I'd preferably use implode() instead of join() just because it is the actual and more commonly used PHP function, and since it aligns better with its counterpart explode().
  • Last but not least, let's look at the name of the filter ms_user_list_site_class for a bit longer. I'm not sure whether it's appropriate/precise enough. I'm not opposed to the current name, just something to think about further.

#5 @kraftbj
7 years ago

Cool, I'll make those changes.

I went with join vs implode after copying the body_class function. I couldn't recall/find if there was a preference in Core. I'll switch it over since, in the end, I want the committer happy with whatever they're committing ;-)

For the length of the filter name, I'm apathetic. Tried to mimic the naming used elsewhere ms_user_list_site_actions (noting how bad of an idea to follow MS naming schemes), but yeah, no preference on my end.

@kraftbj
7 years ago

updated docs and other suggested changes

This ticket was mentioned in Slack in #core-multisite by kraft. View the logs.


7 years ago

#7 @kraftbj
7 years ago

  • Milestone changed from Awaiting Review to 5.0

Moving this into 5.0 to ensure consideration.

#8 @peterwilsoncc
6 years ago

  • Milestone changed from 5.0 to 5.1

Switching milestone due to the focus on the new editor (Gutenberg) for WordPress 5.0.

#9 @pento
6 years ago

  • Milestone changed from 5.1 to 5.2

This ticket was mentioned in Slack in #core-multisite by kraft. View the logs.


6 years ago

#11 @johnjamesjacoby
6 years ago

I am +1 to this change.

I think the patch is acceptable as is (given the current standard of how core handles this elsewhere.)


That said, here is what I wish core would enforce (in more places than this):

  • The ms_user_list_site_class filter may not return an array, but the line of code immediately after the filter is applied trusts that it will be by using array_unique() and implode() on it. I'd like to cast the results of apply_filters() as an (array) to avoid errors.
  • If $site_classes is empty, a single space will be used in the class attribute. This is not valid.
  • sanitize_html_class() exists, but isn't used in so many places. Right now, $site_classes could include invalid strings, or quotes that break out of the HTML attribute entirely. I'd like for each class in the array to be properly sanitized.
  • esc_attr( implode( ' ', array_unique( $site_classes ) ) ) is 3 nested, inline, function calls; it feels excessive to me (when not inside a dedicated helper/convenience function.) Core does not have a standard for this, but 2 feels like the max before it gets too busy for the logical order of things to be clearly understood.

@kraftbj
6 years ago

Addressed JJJ's comments.

#12 @kraftbj
6 years ago

Thanks @johnjamesjacoby and my apologies for overlooking the notification.

I agree on all fronts. I refreshed the patch since it didn't apply cleanly anymore and added:

  1. array_map with sanitize_html_class the values before output.
  2. moved the array_unique to have two twice-nested calls instead of the one with three.
  3. conditional that verified that the array was an array and was not empty
  4. left if outputting a span to avoid complicating the closing and to preserve the same DOM tree as some defensive BC for someone CSSing based on the element, not classes/ids.

#13 @desrosj
6 years ago

  • Owner changed from kraftbj to desrosj

#14 @desrosj
6 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 44977:

Networks and Sites: Introduce the ms_user_list_site_class filter.

In the network admin user table on multisite installs (WP_MS_Users_List_Table), this filter allows the classes for the <span> tag surrounding each site link to be modified.

Props kraftbj, flixos90, johnjamesjacoby.
Fixes #41146.

#15 @SergeyBiryukov
6 years ago

In 45252:

Networks and Sites: Use correct escaping function for classes added via ms_user_list_site_class filter.

Props david.binda.
Fixes #46990. See #41146.

Note: See TracTickets for help on using tickets.