WordPress.org

Make WordPress Core

Opened 40 hours ago

Last modified 8 hours ago

#54504 reopened task (blessed)

Update Requests library to version 2.0.0

Reported by: jrf Owned by: SergeyBiryukov
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: External Libraries Keywords: has-patch needs-dev-note php80 php81
Focuses: Cc:

Description

The Requests library has released a new major version: 2.0.0.

This is a major release and contains breaking changes.

Most important changes to be aware of for this release:

  • All code is now namespaced, though there is a full BC-layer available and the old class names are still supported, though using them will generate a deprecation notice (which can be silenced by plugins if they'd need to support multiple WP versions). An upgrade guide is available and I'd recommend for this change + a link to the upgrade guide to be included in the WP 5.9 dev-note.
  • A lot of classes have been marked final. This should generally not affect userland code as care has been taken to not apply the final keyword to classes which are known to be extended in userland code.
  • Extensive input validation has been added to Requests. When Requests is used as documented though, this will be unnoticable.
  • A new WpOrg\Requests\Requests::has_capabilities() method has been introduced which can be used to address #37708
  • A new WpOrg\Requests\Response::decode_body() method has been introduced which may be usable to simplify some of the WP native wrapper code.
  • Remaining PHP 8.0 compatibility fixed (support for named parameters)
  • PHP 8.1 compatibility

Full changelog: https://github.com/WordPress/Requests/releases/tag/v2.0.0

Website (updated): https://requests.ryanmccue.info/

It is recommended for WordPress to update the bundled version of Requests.

I've prepared a PR for the update and will link it to this ticket.

Previous: #33055, #47746, #49922, #53101, #53334

Change History (15)

#1 @jrf
40 hours ago

  • Keywords php80 php81 added

This ticket was mentioned in PR #1624 on WordPress/wordpress-develop by jrfnl.


40 hours ago

  • Keywords has-unit-tests added

### External libraries: upgrade to Requests 2.0.0

### Upgrade references to the Requests library in the WP code

Trac ticket: https://core.trac.wordpress.org/ticket/54504

#3 @jrf
40 hours ago

  • Keywords has-unit-tests removed

GH PR is ready for review and if found okay, for commit: https://github.com/WordPress/wordpress-develop/pull/1624

#4 @SergeyBiryukov
40 hours ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#5 @prbot
39 hours ago

jrfnl commented on PR #1624:

Marked as ready for review and updated with the final Requests 2.0.0 code.

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


35 hours ago

#7 @SergeyBiryukov
30 hours ago

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

In 52244:

External Libraries: Update the Requests library to version 2.0.0.

This is a major release and contains breaking changes.

Most important changes to be aware of for this release:

  • All code is now namespaced. Though there is a full backward compatibility layer available and the old class names are still supported, using them will generate a deprecation notice (which can be silenced by plugins if they'd need to support multiple WP versions). See the upgrade guide for more details.
  • A lot of classes have been marked final. This should generally not affect userland code as care has been taken to not apply the final keyword to classes which are known to be extended in userland code.
  • Extensive input validation has been added to Requests. When Requests is used as documented though, this will be unnoticable.
  • A new WpOrg\Requests\Requests::has_capabilities() method has been introduced which can be used to address #37708.
  • A new WpOrg\Requests\Response::decode_body() method has been introduced which may be usable to simplify some of the WP native wrapper code.
  • Remaining PHP 8.0 compatibility fixed (support for named parameters).
  • PHP 8.1 compatibility.

Release notes: https://github.com/WordPress/Requests/releases/tag/v2.0.0

For a full list of changes in this update, see the Requests GitHub:
https://github.com/WordPress/Requests/compare/v1.8.1...v2.0.0

Follow-up to [50842], [51078].

Props jrf, schlessera, datagutten, wojsmol, dd32, dustinrue, soulseekah, costdev, szepeviktor.
Fixes #54504.

#8 follow-up: @dd32
29 hours ago

Can we consider renaming the namespace from WpOrg to something more generic, such as WordPress or WP?

That appears to be the first WordPress-specific namespace used within WordPress, and setting the name as WpOrg seems like it might be rather ugly later on.

The inclusion of Org is also kind of close to the namespace used on WordPress.org of WordPressdotorg.. So if this was intended on being "Not WordPress, but a WordPress.org thing" then that longer format may have been more appropriate, I'm not sure.

I realise this has been something that has been decided "upstream" in Requests, but raising this here since it's where the merge has happened.

#9 in reply to: ↑ 8 @SergeyBiryukov
29 hours ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to dd32:

Can we consider renaming the namespace from WpOrg to something more generic, such as WordPress or WP?

That appears to be the first WordPress-specific namespace used within WordPress, and setting the name as WpOrg seems like it might be rather ugly later on.

Thanks for bringing this up! That was my concern here too, but I didn't want it to block the merge.

The inclusion of Org is also kind of close to the namespace used on WordPress.org of WordPressdotorg.. So if this was intended on being "Not WordPress, but a WordPress.org thing" then that longer format may have been more appropriate, I'm not sure.

I realise this has been something that has been decided "upstream" in Requests, but raising this here since it's where the merge has happened.

For reference, it appears to be implemented in this PR: Requests 2.0.0: introduce namespaces:

Acronyms are "proper cased", i.e. first character uppercase, the rest of the acronym lowercase.

See this comment specifically for more details.

Either WordPressdotorg or WPorg, or simply WordPress or WP would be my preference too, though it slightly deviates from the "proper cased" rule used for other acronyms. Could it perhaps be an exception?

Reopening for discussion.

Last edited 29 hours ago by SergeyBiryukov (previous) (diff)

#10 @johnbillion
28 hours ago

  • Type changed from defect (bug) to task (blessed)

#11 @costdev
28 hours ago

Using something like WordPress makes sense to me too. As it isn't an acronym, it should be fine to use CamelCaps.

WpOrg (lowercase or uppercase) is also a commonly used abbreviation for wordpress.org.

It's possible that WpOrg was used in an effort to say "This is created under the WordPress Organisation on GitHub". In this case, using WordPress may incorrectly imply that WordPress is a dependency for Requests.

It would be good to get some insight into the reason(s) why WpOrg was chosen or, more specifically, why WordPress wasn't chosen. It's quite possible that we're unaware of sound reasoning for it that we should consider when discussing an alternative namespace.

#12 follow-up: @jrf
19 hours ago

@dd32 I appreciate the concern, but Requests 2.0.0 has been released and the discussion about the namespace was had publicly in that repo, in open videocalls and in WP Slack months ago.
Input at the time would have been very welcome, but was not received and it's too late to change this now.

The choice for the namespace was discussed extensively and various options were considered during those discussions.

We originally were going to use just Requests\ as the "prefix", but that was causing problems with the BC-layer. In our opinion, including the BC-layer was imperative to allow for plugins supporting multiple versions of WP and using parts of Requests directly, so that meant we had to reconsider the namespace.

We considered using WordPress\Requests\, but as Requests is a stand-alone project, we did not want to create a situation were the namespace chosen by Requests could ever become a blocker for WordPress itself adopting namespaces at some point in the future.

We also considered that Requests is used by more projects than just WordPress and that the namespace prefix should not cause undue friction in other projects using Requests, so we ended up compromising on WpOrg\Requests\.

While it is, of course, perfectly possible to change it for the copied version included in WP, this would make all future merges of Requests a lot more complex, so I'd strongly recommend against that.

#13 in reply to: ↑ 12 @SergeyBiryukov
16 hours ago

Replying to jrf:

We also considered that Requests is used by more projects than just WordPress and that the namespace prefix should not cause undue friction in other projects using Requests, so we ended up compromising on WpOrg\Requests\.

Thanks for the context! Since PHP namespaces are case-insensitive, is there any chance to capitalize that as WPorg\Requests\, so that the capitalization of WP is consistent in lines like this and across the project in general?

new WP_REST_Request( WPorg\Requests\Requests::POST, $route );
Last edited 11 hours ago by SergeyBiryukov (previous) (diff)

#14 @jrf
15 hours ago

Since PHP namespaces are case-insensitive, is there any chance to capitalize that as WPorg\Requests\, so that the capitalization of WP is consistent in lines like this and across the project in general?

@SergeyBiryukov Good question. We did actually discuss that as well.

For the PSR-4 class name to path translation in the autoloaders (both the supported Composer autoloader as well as the custom autoloader), the namespace and class names have to be treated case-sensitively as otherwise this would break when code is run on case-sensitive OS-es, like *nix and MacOS.

To then treat the namespace prefix case-insensitively would be inconsistent, especially as this could only be supported when using the custom autoloader, but would break when using the Composer autoloader, which is why it was decided to not support that.

Last edited 11 hours ago by SergeyBiryukov (previous) (diff)

#15 @prbot
8 hours ago

jrfnl commented on PR #1624:

Closing as committed via changeset 52244.

Note: See TracTickets for help on using tickets.