Making WordPress.org

Opened 5 years ago

Closed 5 years ago

#4758 closed defect (bug) (fixed)

Adjust function signature of WPorg_Handbook_Walker::walk()

Reported by: jrf's profile jrf Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Priority: high
Component: Handbooks Keywords: has-patch
Cc:

Description

Currently, one of the last patches for WP 5.3 is blocked by a change which needs to be made in meta.

See: https://core.trac.wordpress.org/ticket/47678#comment:72

The change itself is rather simple - basically the same changes as shown in this patch: https://core.trac.wordpress.org/attachment/ticket/47678/47678-Walker-walk.patch

Unfortunately I don't have access to meta, so can't prepare a patch.

The change needs to be made in this file:
https://meta.trac.wordpress.org/browser/sites/trunk/wordpress.org/public_html/wp-content/plugins/handbook/inc/walker.php#L31

Change History (6)

This ticket was mentioned in Slack in #core-committers by jrf. View the logs.


5 years ago

#2 @Otto42
5 years ago

Seems like you have this the wrong way around here.

This piece is a plugin. We don't change plugins before we change core. This change cannot go in until core makes its changes accordingly. Now, we might have to break wp.org to do it, but core changes come before meta changes.

The other way around makes no sense, and it also makes no sense that core changes can be blocked by meta changes, especially ones to the handbook plugin, of all things.

In addition, looking at the code change proposed, I don't really understand it. The syntax must be some of that newfangled PHP 7 stuff or something, but what is the purpose of adding ...$args instead of just using func_get_args()? The func_get_args function isn't deprecated or removed. This whole thing seems like an unnecessary change that breaks backward compatibility for no reason or benefit.

#3 @jrf
5 years ago

@Otto42 Thanks, but no.

The change originally went in and was reverted as it caused a lot of warnings to be thrown for dotorg.

Commit: https://core.trac.wordpress.org/changeset/45624
Revert: https://core.trac.wordpress.org/changeset/45640

It has been agreed that the change should go into WP 5.3, but to prevent the same load of warnings being thrown, the #meta change should be made at the same time.

In addition, looking at the code change proposed, I don't really understand it.

May I suggest educating yourself by reading the original ticket ?

Last edited 5 years ago by jrf (previous) (diff)

#4 @Otto42
5 years ago

You're kind of missing the point here, I feel.

Yes, a change to core will break org, and thus we correct org afterwards to correct the problem. This happens whenever you break backward compatibility.

But that works either way. If we change the code on .org before changing core, then we get the same problems and warnings. You don't solve a compatibility break by changing it in one place over another. Both changes need to happen simultaneously.

May I suggest educating yourself by reading the original ticket ?

May I suggest that you simply explain the issue in detail here in this ticket, instead of pointing to a 3 month old ticket with 70+ comments and 50+ patch attachments?

Because I'm not going to make this change without some explanations. If this means core is blocked, then core will stay blocked.

#5 @jrf
5 years ago

If we change the code on .org before changing core, then we get the same problems and warnings.

Not true. The change to #meta is BC-compatible with older WP versions (pre the core patch going in).

May I suggest that you simply explain the issue in detail here in this ticket, instead of pointing to a 3 month old ticket with 70+ comments and 50+ patch attachments?

Well, without repeating myself overly much, please read this, which is the draft dev-note and explains both the changes as well as why changing #meta first won't cause an issue: https://gist.github.com/jrfnl/23fa057e5a951ab8b6bf8709e5e5b281

#6 @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 9166:

Handbook: Update WPorg_Handbook_Walker::walk() signature for compatibility with the upcoming WordPress core change in [WP46442], introducing the spread operator in Walker::walk().

Props jrf.
Fixes #4758. See #WP47678.

Note: See TracTickets for help on using tickets.