WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 4 months ago

Last modified 4 months ago

#48267 closed defect (bug) (fixed)

IXR: Use spread args (was Modernize IXR library)

Reported by: kraftbj Owned by:
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: XML-RPC Keywords: has-dev-note
Focuses: Cc:

Description

Motivated by #47678, I started updating func_get_args calls in a plugin that extended the IXR classes provided by Core and realized that I was getting ahead of myself and Core.

With this coming from a seemingly abandoned upstream library where we haven't modernize the code in some time, before doing more (PHPCS, inline docs, etc), what is the status of this part of the code? Open to being modernized?

Attaching a simple patch for the spread operator to start the discussion.

Subtasks:

  • Remove PHP 4 and pre-PHP 5.6 conventions.
  • PHPCS.
  • Inline documentation.

Related: #46484

Attachments (3)

48267.diff (1.4 KB) - added by kraftbj 13 months ago.
Add spread operator
48267-lsp-fixes.patch (554 bytes) - added by ayeshrajans 4 months ago.
48267-lsp-fixes.2.patch (588 bytes) - added by ayeshrajans 4 months ago.

Download all attachments as: .zip

Change History (16)

@kraftbj
13 months ago

Add spread operator

#1 @SergeyBiryukov
13 months ago

  • Milestone changed from Awaiting Review to 5.4

#2 in reply to: ↑ description @SergeyBiryukov
12 months ago

Replying to kraftbj:

With this coming from a seemingly abandoned upstream library where we haven't modernize the code in some time, before doing more (PHPCS, inline docs, etc), what is the status of this part of the code? Open to being modernized?

Looking at the recent commits, of which [38389] is probably the most substantial:

it appears to be treated as "adopted" rather than external, so I think we should bring it in line with the rest of core.

#3 @audrasjb
8 months ago

  • Milestone changed from 5.4 to Future Release

Hi,

With 5.4 Beta 3 approaching and the Beta period reserved for bugs introduced during the cycle, this is being moved to Future Release. If any maintainer or committer feels this should be included or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#4 @SergeyBiryukov
4 months ago

  • Milestone changed from Future Release to 5.5

#5 @SergeyBiryukov
4 months ago

In 48204:

Code Modernization: Introduce the spread operator in wp-includes/IXR.

Rather than relying func_get_args() to retrieve arbitrary function arguments, we can now use the spread operator to assign them directly to a variable.

Props kraftbj.
See #48267, #47678.

#6 @SergeyBiryukov
4 months ago

  • Keywords needs-dev-note added

[48204] should be mentioned in a dev note, maybe with a link to the previous dev note from #47678 for more context.

Keeping the ticket open for now to address the other subtasks from the description.

Last edited 4 months ago by SergeyBiryukov (previous) (diff)

#7 @ayeshrajans
4 months ago

Please note that this breaks PHP 8 builds because PHP 8 enforce strict LSP checks: https://php.watch/versions/8.0/lsp-errors

Build: https://travis-ci.com/github/Ayesh/wordpress-develop/jobs/355902441#L2647

In versions prior to PHP 8, this change still raises a warning, so I think this needs to be fixed before 5.5 even though we target 5.6 for PHP 8.

This ticket was mentioned in Slack in #core by ayesh. View the logs.


4 months ago

#9 @SergeyBiryukov
4 months ago

In 48238:

Code Modernization: Introduce the spread operator in WP_HTTP_IXR_Client.

Rather than relying func_get_args() to retrieve arbitrary function arguments, we can now use the spread operator to assign them directly to a variable.

This makes the signature of WP_HTTP_IXR_Client::query() compatible with the parent class method.

Follow-up to [48204].

Props ayeshrajans.
See #48267, #47678.

#10 @ayeshrajans
4 months ago

Thanks a lot for quickly picking this up @SergeyBiryukov 🙏🏿.

#11 @desrosj
4 months ago

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

I am in the process of drafting the dev note and just confirmed this is in the list of notes being tracked by the release squad. Going to close this out to help clear the milestone in advance of beta 1 tomorrow.

#12 @kraftbj
4 months ago

  • Summary changed from IXR: Modernize IXR library to IXR: Use spread args (was Modernize IXR library)

Thanks @desrosj. I'll open up a new ticket to track more specific pieces of this to save the need to juggle tickets moving forward.

#13 @desrosj
4 months ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.