WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#40503 closed enhancement (fixed)

Use get_network_option in wpmu_create_blog

Reported by: spacedmonkey Owned by: flixos90
Milestone: 4.9 Priority: normal
Severity: normal Version: 3.0
Component: Networks and Sites Keywords: has-patch has-unit-tests
Focuses: multisite Cc:

Description

In wpmu_create_blog, you can pass a site id, however this is not passed to the following file

'WPLANG' => get_site_option( 'WPLANG' )

Attachments (3)

40503.diff (2.2 KB) - added by spacedmonkey 4 years ago.
40503.2.diff (4.6 KB) - added by spacedmonkey 3 years ago.
40503.3.diff (3.4 KB) - added by flixos90 3 years ago.

Download all attachments as: .zip

Change History (14)

@spacedmonkey
4 years ago

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


4 years ago

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


4 years ago

#3 @flixos90
4 years ago

  • Keywords early has-patch added
  • Milestone changed from Awaiting Review to Future Release

#4 @spacedmonkey
3 years ago

  • Milestone changed from Future Release to 4.9
  • Owner set to flixos90
  • Status changed from new to assigned

#5 @flixos90
3 years ago

  • Keywords needs-unit-tests added; early removed
  • Status changed from assigned to reviewing

The actual change in the patch looks good.

Changing the variable name from $site_id to $network_id is not really a practice we've been following, but recent efforts in ensuring consistency with our coding standards give me a feeling that this becomes more acceptable. I'm not entirely sure the name change belongs in here, but I'm not opposed either.

A unit test will be necessary though.

@spacedmonkey
3 years ago

#6 @spacedmonkey
3 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Tests added

#7 @DrewAPicture
3 years ago

@spacedmonkey Looks pretty good.

Now that we're (finally!) renaming $site_id to $network_id in a couple places, we should write more useful descriptions where they're referenced in DocBlocks. "Optional. Only relevant on multi-network installs." has always been pretty meh :-)

#8 follow-up: @flixos90
3 years ago

@DrewAPicture I take that as a confirmation that changing variable names like that to follow current naming conventions is acceptable. :)

#9 in reply to: ↑ 8 @DrewAPicture
3 years ago

Replying to flixos90:

@DrewAPicture I take that as a confirmation that changing variable names like that to follow current naming conventions is acceptable. :)

I don't have a problem with it, as long as it's improving the ability for the code to better self-dccument. The $site_id vs $network_id thing is super confusing in a bunch of places – it would be good to provide some clarity here and elsewhere.

@flixos90
3 years ago

#10 @flixos90
3 years ago

40503.3.diff is a tiny update on the previous patch.

Having a second look, I think it doesn't make much sense to change the $site_id to $network_id here, since there is still $blog_id around, and having a function with $blog_id and $network_id is worse in my opinion. It would also be strange to rename $blog_id to $site_id since the function would still have "blog" in the name.

The rename here is not worth the changeset clutter, although I generally like the idea of doing such changes. Given that we're working on a better implementation of a similar method anyway (see #40364), I think we can leave things as they are for now.

I only changed a bit of documentation to use the correct "Site" and "Network" terms there.

#11 @flixos90
3 years ago

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

In 41058:

Multisite: Use get_network_option() for language in wpmu_create_blog().

Before this changeset, the language of a new site would always result in the language of the current network, regardless of the $site_id parameter passed that actually determines the network for the site. Now the correct WPLANG value is used in such cases.

Alongside this change, a few minor documentation changes around the function have been made to account for the current naming conventions of sites and networks.

Props spacedmonkey.
Fixes #40503.

Note: See TracTickets for help on using tickets.