WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#41510 closed enhancement (fixed)

Rename site_id to network_id in multisite functions

Reported by: spacedmonkey Owned by: lemacarl
Milestone: 4.9 Priority: normal
Severity: normal Version: 3.0
Component: Networks and Sites Keywords: good-first-bug has-patch commit
Focuses: docs, multisite Cc:

Description

In many multisite functions use variable named $site_id which references the id of the network. This is confusing, as the current naming in multisite is network and site. In an effort have better self documentation, these variables should be changed.

Attachments (4)

41510.patch (4.6 KB) - added by lemacarl 3 years ago.
41510.2.patch (6.7 KB) - added by lemacarl 3 years ago.
41510.3.patch (11.6 KB) - added by lemacarl 3 years ago.
41510.diff (12.3 KB) - added by flixos90 3 years ago.

Download all attachments as: .zip

Change History (23)

#1 @spacedmonkey
3 years ago

Looping @DrewAPicture into this one, as it is docs related.

#2 @DrewAPicture
3 years ago

@spacedmonkey Care to work up a patch?

@jeremyfelt @flixos90 What's your take here?

#3 @spacedmonkey
3 years ago

@DrewAPicture I mark this as a good first bug, as I want to give it to a newbe. If the ticket is here at the end of 4.9, I will make up a patch.

#4 @flixos90
3 years ago

I think it's a good idea to change $site_id to $network_id everywhere. Even more important than the variables though, there are a few occurrences even in doc-blocks where the term "site" is used where it should be "network". Maybe that could happen through this ticket as well.

I'd like to highlight though that at this point let's not change $blog_id to $site_id. This should definitely happen at some point, but not before the term "site" is not used for a network anywhere anymore. Furthermore we need some more site-related functions with "site" instead of "blog" in the name for it to make sense (this is something that is underway, but happens organically over time).

To summarize, in the near future a world of $blog_id/$site_id for sites and $network_id for networks is the thing to aim for. This at least clears up the double meaning for the term "site".

#5 @lemacarl
3 years ago

@spacedmonkey It looks like this bug also extends to the DB because the column name site_id is also used instead of network_id which is how it is used by insert_blog in multisite functions. In fact the Codex explicitly states that the the site_id identifies the network. In which case this bug would cover the entire problem. How do I proceed?

#6 follow-up: @flixos90
3 years ago

@lemacarl Unfortunately the database schema is nothing we can change here. In those cases, it must remain as is.

#7 in reply to: ↑ 6 @lemacarl
3 years ago

@flixos90 Ok. Thanks.

@lemacarl
3 years ago

#8 @lemacarl
3 years ago

  • Keywords has-patch added; needs-patch removed

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


3 years ago

#10 @spacedmonkey
3 years ago

  • Milestone changed from Awaiting Review to 4.9

#11 @flixos90
3 years ago

  • Owner set to lemacarl
  • Status changed from new to assigned

@lemacarl It's only a minor issue, but could you make sure that you update the indentation of the docblocks? Now that the parameter names have been changed from $site_id to the longer $network_id, the following descriptions are not in line with the other parameters anymore.

@lemacarl
3 years ago

#12 @lemacarl
3 years ago

@flixos90 I've aligned the docblocks in the new patch

#13 follow-ups: @spacedmonkey
3 years ago

Place that were missed.

wp-db.php
public function set_blog_id( $blog_id, $site_id = 0 ) {

if ( ! empty( $site_id ) )

$this->siteid = $site_id;

can_edit_network( $site_id )

ms-settings.php

$site_id = $current_blog->site_id;
wp_load_core_site_options( $site_id );

option.php
wp_load_core_site_options
ms-deprecated.php
get_admin_users_for_domain

@lemacarl do you want to update the patch with these other places.

#14 in reply to: ↑ 13 @flixos90
3 years ago

Replying to spacedmonkey:

ms-settings.php

$site_id = $current_blog->site_id;
wp_load_core_site_options( $site_id );

Careful: That occurrence must not be changed, since it globalizes the variable. Changing this is part of #41285.

#15 in reply to: ↑ 13 @lemacarl
3 years ago

@spacedmonkey Cool. On it

@lemacarl
3 years ago

#16 @spacedmonkey
3 years ago

Looks good to me.

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


3 years ago

@flixos90
3 years ago

#18 @flixos90
3 years ago

  • Keywords commit added

@lemacarl Thanks for following up on this. I fixed some spacing issues in 41510.diff. Be aware that per the coding standards, spaces should be used for indenting parameter documentation. The return documentation doesn't need to be aligned with the parameter indentation.

#19 @flixos90
3 years ago

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

In 41241:

Multisite: Rename internal $site_id variables referencing networks to $network_id.

This change improves code clarity by using the current naming conventions for networks.

Props lemacarl.
Fixes #41510.

Note: See TracTickets for help on using tickets.