WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 2 months ago

#45594 closed enhancement (fixed)

Action 'switch_blog' duplicated $new_blog param

Reported by: ChriCo Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.0
Component: Networks and Sites Keywords: has-patch needs-testing
Focuses: multisite Cc:
PR Number:

Description

The action 'switch_blog' which can be found in wp-includes/ms-blogs.php L1660 has two times $new_blog injected, while the Doc Block says:

<?php
* @param int $new_blog New blog ID.
* @param int $new_blog Blog ID.

This is maybe - in this context - correct, since in L1651 we're checking for if ( $new_blog == $blog_id ) before doing the action.

But regarding correctness, it should be $new_blog and $prev_blog_id (as declared after the if-statement) as param and in docBlocks (for generation of documentation).

My patch...

  1. renames $blog_id into $prev_blog_id.
  2. updates the docBlock for the action 'switch_blog'.
  3. update third param in 'switch_blog' to $prev_blog_id.
  4. removes the not required $prev_blog_id = $blog_id.

Attachments (1)

45594.patch (1.5 KB) - added by ChriCo 11 months ago.

Download all attachments as: .zip

Change History (9)

@ChriCo
11 months ago

#1 @swissspidy
11 months ago

  • Focuses multisite removed
  • Keywords has-patch needs-testing added

#2 follow-up: @jeremyfelt
10 months ago

  • Focuses multisite added
  • Milestone changed from Awaiting Review to Future Release

Thanks, @ChriCo, some consistency in docs and parameters makes sense here. I'm wondering if we can do this with a smaller change.

The $prev_blog_id parameter was populated with $GLOBALS['blog_id'] until [38457]. It now serves less of a purpose because there is no worry of the global being overwritten on the next line.

I think we can remove $prev_blog_id entirely and change both action calls to do_action( 'switch_blog', $new_blog, $blog_id );

#3 in reply to: ↑ 2 @ChriCo
10 months ago

Replying to jeremyfelt:

I think we can remove $prev_blog_id entirely and change both action calls to do_action( 'switch_blog', $new_blog, $blog_id );

Thanks for your feedback. My intention was to name it in way everyone can understand it and also in terms of automatic generation of documentation out of docblocks. The var name $blog_id is not very exact here. Is it the current, new or previous one?

Last edited 10 months ago by ChriCo (previous) (diff)

#4 @ChriCo
10 months ago

Howdy,

any news here? The patch is working so far, so it needs only an "approved" or "not approved".

#5 @ChriCo
2 months ago

@jeremyfelt any news here? This is already over a half year ago and just a small "fix" in terms of rename internal used variables and update of a DocBlock of the action switch_blog. Any blocker here?

#6 @SergeyBiryukov
2 months ago

  • Milestone changed from Future Release to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#7 @SergeyBiryukov
2 months ago

Hi there! Thanks for the ticket and the patch, sorry it took so long for someone to get back to you.

The proposed changes make sense to me, both the documentation fix and the clarified naming.

Let's do the same in restore_current_blog() for consistency.

#8 @SergeyBiryukov
2 months ago

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

In 45794:

Networks and Sites: Improve documentation and variable naming in switch_to_blog() and restore_current_blog().

In switch_to_blog():

  • Rename $blog_id to $prev_blog_id for clarity.
  • Rename $new_blog to $new_blog_id for consistency.
  • Pass $prev_blog_id as a second parameter to switch_blog action, instead of the duplicated $new_blog_id. This only clarifies documentation and does not affect functionality, since the values are equal in the context where the DocBlock is located.

In restore_current_blog():

  • Rename $blog to $new_blog_id for clarity.
  • Rename $blog_id to $prev_blog_id for clarity.

Props ChriCo, jeremyfelt, SergeyBiryukov.
Fixes #45594.

Note: See TracTickets for help on using tickets.