WordPress.org

Make WordPress Core

Opened 2 weeks ago

Last modified 32 hours ago

#54504 reopened task (blessed)

Update Requests library to version 2.0.0

Reported by: jrf Owned by: SergeyBiryukov
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: External Libraries Keywords: php80 php81 has-patch has-unit-tests early early-like-actually-early
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

Attachments (1)

php-namespaces-advantages.png (39.7 KB) - added by azaozz 44 hours ago.
Advantages of namespacing as per the PHP manual. Frankly I see both as being a disadvantages in some cases. Overriding internal funstion and class names would be super confusing for anybody trying to read the code. "Aliasing" long, descriptive names to short non-descriptive is... a bad thing, isn't it? :)

Download all attachments as: .zip

Change History (42)

#1 @jrf
2 weeks ago

  • Keywords php80 php81 added

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


2 weeks 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
2 weeks 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
2 weeks ago

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

#5 @prbot
2 weeks 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.


2 weeks ago

#7 @SergeyBiryukov
2 weeks 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
2 weeks 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
2 weeks 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 2 weeks ago by SergeyBiryukov (previous) (diff)

#10 @johnbillion
2 weeks ago

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

#11 @costdev
2 weeks 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
2 weeks 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
13 days 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 13 days ago by SergeyBiryukov (previous) (diff)

#14 @jrf
13 days 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 13 days ago by SergeyBiryukov (previous) (diff)

#15 @prbot
13 days ago

jrfnl commented on PR #1624:

Closing as committed via changeset 52244.

#16 @dd32
10 days ago

while I still disagree with the namespacing choice chosen here, don't believe that the choices taken were correct (even if "publicly discussed"), and still strongly feel like Requests is using a completely wrong namespace choice (Which despite R2 being released, could be changed)

I'm happy to agree that the choice has been made and it's too late to really change that. I would like to see the Namespace properly capitalised though.

#17 @SergeyBiryukov
9 days ago

Similar feelings here, I would expect any naming changes that affect core and backward compatibility to be brought up in a dev chat for more feedback. I appreciate that one may not always have time or energy for that though.

#18 @jrf
9 days ago

Now look:

  1. Requests is still an external dependency. Do you expect SimplePie or PHPMailer to bring something like this up in the WP Core Slack ? No, so don't expect it from Requests either.
  2. The roadmap for Requests 2.0.0 was published well in time, the PR for the namespacing was open for more than a month, the release cycle was something like six months, with the release being delayed for an extra two months as well. Any feedback before the actual release would have been very welcome and would have been taken into account.
  3. Changing the namespace now, even if just the case of the namespace, IS a breaking change, what with the autoloading being case-sensitive, and would require a 3.0 release.

The repo is public, conversations were had in public, there is a even a #core-http-api channel in the WP Slack for transparency for those interested, but nobody showed an interest.

It's easy to complain and criticize after the fact. If you wanted to have a say, you should have gotten involved.

#19 follow-ups: @schlessera
9 days ago

As @jrf already stated, Requests is still considered an external, third-party dependency, which moved from the ryanmccue personal GitHub account to the wordpress organization GitHub account. Requests has never adhered to the WordPress Core coding style. And now it has adopted PSR-4 and most PSR-12 conventions.

The correct way to name it according to the code style in place in the repository and the conventions in place in the larger PHP ecosystem would have been to use Wordpress\Requests (note the lower-case P, part of the convention for PSR-4). We wanted to avoid that for obvious reasons, as Requests is neither a part of the actual WordPress CMS, nor did we want to set a precedent for namespacing Core. What we have now is therefore under the 'wordpress' GH org as a namespace.

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 );

The above would actually be expected to use an import, which would turn the code into:

new WP_REST_Request( Requests::POST, $route );

The import itself can be automatically generated/managed by any decent IDE or code quality tool, so you would never type that namespace anyway.

Case-insensitive class names are often an issue if you work on multiple filesystems because any string comparisons to deal with the namespace (most notably in autoloading) tend to break. That's why most code quality tools will flag these as errors, and Composer explicitly does not even support them. I wouldn't be surprised if PHP decided to make them case-sensitive in a later version, as they are slowly getting rid of all and any ambiguities that might introduce bugs.

I see no reason why the class name would be problematic, as with proper usage, it becomes inconsequential.

Also, just as a final remark, @jrf and me (well @jrf, mostly) invested lots of time and energy in making a library that is a critical dependency of WordPress Core but had fallen into disrepair usable and maintainable again. A lot of thought went into everything, to ensure it works both as a dependency of WordPress Core as well as a useful package for any other PHP project that is depending on it, while maintaining backwards compatibility whenever we could. The entire endeavour was planned in such a way that pulling in the latest version is as frictionless as possible and can mostly just be automated. It would now be a total waste of contributor time to make changes just for the sake of it. However, I understand people feel strongly about the "WP brand/trademark". But I for one will not accept this additional effort be taken on by the Requests package. As a consumer of the Requests library, WordPress Core is of course free to render the process as efficient/inefficient as it wants, but I don't see any valid reason why the Requests package should adapt to that.

#20 in reply to: ↑ 19 @SergeyBiryukov
9 days ago

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

Replying to schlessera:

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 );

The above would actually be expected to use an import, which would turn the code into:

new WP_REST_Request( Requests::POST, $route );

The import itself can be automatically generated/managed by any decent IDE or code quality tool, so you would never type that namespace anyway.

Thanks for chiming in! That was just one of the examples of the current code as committed in [52244]:

new WP_REST_Request( WpOrg\Requests\Requests::POST, $route );

There are currently ~60 instances like that in core, though I guess they could use an import too.

Also, just as a final remark, @jrf and me (well @jrf, mostly) invested lots of time and energy in making a library that is a critical dependency of WordPress Core but had fallen into disrepair usable and maintainable again. A lot of thought went into everything, to ensure it works both as a dependency of WordPress Core as well as a useful package for any other PHP project that is depending on it, while maintaining backwards compatibility whenever we could. The entire endeavour was planned in such a way that pulling in the latest version is as frictionless as possible and can mostly just be automated.

There is no doubt about that, I highly appreciate all the efforts what went into the 2.0.0 release, and I'm sure that most users of the package will too.

I guess I do feel a bit strongly about capitalizing the WP acronym in a consistent way, but I'm happy to accept that the decision has been made and will not be addressed further. Re-closing this as fixed, as it looks like a consensus was reached here.

#21 @SergeyBiryukov
7 days ago

Follow-up: #54546, #54547.

Last edited 7 days ago by SergeyBiryukov (previous) (diff)

#22 @hellofromTonya
2 days ago

  • Keywords needs-patch added; has-patch needs-dev-note removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to revert Requests 2.0.0 and move introducing it to the 6.0.0 cycle. Why?

The namespace and file case renaming in Requests 2.0.0 revealed 2 fundamental issues in the upgrader process:

  • Not all of the current "from" WordPress version code is preloaded into memory before the upgrader changes the filesystem. See #54546 and #54589.
  • The Filesystem does not identify or handle filename case changes on case-insensitive filesystems (see #54582 and #54547). This impacts Core and WP-CLI. It means on those systems the files are (a) not renamed and (b) are deleted when the old file removal deletes the files. The result is a fatal error.

These issues are not due to Requests library, but rather are due to the issues above in the filesystem and upgrader.

Reverting Requests back to its 5.8 version does not resolve the above issues but does restore back to 5.8 to avoid risky changes this late in the release cycle. Moving [52244] and [52315] along with the other tickets to 6.0 early gives breathing room and soak time for thoroughly investigating, refining, and multitiered testing.

Marking for needs-patch for reverting both [52244] and [52315]/#54562.

After the revert, then the milestone can be changed to 6.0 and marked as early.

This ticket was mentioned in PR #2016 on WordPress/wordpress-develop by hellofromtonya.


2 days ago

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

Confidence check PR for ensuring all changesets are identified for revert.

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

#24 @hellofromTonya
2 days ago

In 52327:

HTTP API: Revert changeset [52315].

Reverting Requests 2.0.0 changes and moving to WordPress 6.0 cycle. Why? The namespace and file case renaming revealed 2 issues in Core's upgrader process.

See https://core.trac.wordpress.org/ticket/54504#comment:22 for more information.

See #54562, #54504.

#25 @hellofromTonya
2 days ago

  • Keywords early early-like-actually-early added
  • Milestone changed from 5.9 to 6.0

Moving this ticket to 6.0. Once the branch is opened, let's get this committed early-like-actually-early. In doing so, it'll help with the upgrader enhancements for preloading and case-insensitive filesystem detection.

#26 in reply to: ↑ 19 ; follow-up: @azaozz
2 days ago

Replying to schlessera:

As @jrf already stated, Requests is still considered an external, third-party dependency

If it is under the "WordPress umbrella" it should follow the WordPress best practices, i.e. backwards compatibility is a number one priority. Frankly it is unthinkable that a library developed as part of WordPress would not only break back-compat, but will cause fatal errors when used in core...

The correct way to name it according to the code style in place in the repository

If there is such "code style" requirement in that repository it should be removed as incompatible with WordPress. Also, generally, namespaces should be considered "evil" in open source software and avoided as much as possible. The reason it that they promote "lazy" names that go against the best practice in writing good code (poor self-documenting, confusing names, etc.).

Seems a possible/good way forward would be to release Requests 3.0 that will be incompatible with 2.0 but fully compatible with 1.0, then consider it for adding to the next WordPress release. The alternative would be to move the Requests repository out of WordPress and continue developing it as a "true" third-party library that is, unfortunately, incompatible with WordPress.

#27 @SergeyBiryukov
2 days ago

In 52328:

HTTP API: Revert changeset [52244].

Reverting Requests 2.0.0 changes and moving to WordPress 6.0 cycle. Why? The namespace and file case renaming revealed 2 issues in Core's upgrader process.

See https://core.trac.wordpress.org/ticket/54504#comment:22 for more information.

Follow-up to [52327].

See #54562, #54504.

#28 follow-up: @hellofromTonya
2 days ago

Replying to @azaozz:

If it is under the "WordPress umbrella" it should follow the WordPress best practices, i.e. backwards compatibility is a number one priority. Frankly it is unthinkable that a library developed as part of WordPress would not only break back-compat, but will cause fatal errors when used in core...

@azaozz Requests 2.0.0 is fully backwards-compatible and both @jrf and @schlessera made sure that was a priority.

The fatal errors are not due to backwards compatibility, but rather are due to 2 issues identified in my comment above:

  • Detecting and handling case-insensitive filesystems
  • Preloading files into memory before upgrading the filesystem

Requests 2.0.0 revealed both of these upgrader problems. However, Requests itself is not the source of the problems.

Discovering that files must be preloaded means in 6.0 the upgrader can be made more robust and stable to ensure there's no co-mingling of "from" and "to" version code.

Discovering the limitations of not detecting and handling files in case-insensitive filesystems means Core also could never change the case of an existing file. That's limiting. But in 6.0, the Filesystem and/or upgrader could enhanced to detect this and properly handle it.

In talking with the Support team, there's a history of issues with the upgrade process which could be attributed to either of these upgrader issues and will be investigated. I'll also be doing a Trac search to identify other past tickets which might also be part of one or both of these issues.

The discovery of these problems is good as it means the upgrader can be enhanced in 6.0.

#29 @TobiasBg
2 days ago

The original merge changeset [52244] mentioned "PHP 8.1 compatibility".
Should relevant changes (if it's not too many?) maybe be backported to the reverted-to Requests version, to achieve (as much as possible) PHP 8.1 compatibility in WP 5.9 (released two months after PHP 8.1, after all)?

#30 @costdev
2 days ago

@hellofromTonya The discovery of these problems is good as it means the upgrader can be enhanced in 6.0.

+1 to this.

Heavy investigation and testing has been done through the past 5 days by multiple contributors and we have initial drafts of solutions to both problems. When we move to the 6.0 milestone, we'll revisit these to continue discussion and develop the solutions (and unit tests) more fully to get them ready for manual testing early in 6.0.

For now, we're focused back on 5.9 tickets.

#31 in reply to: ↑ 28 ; follow-up: @azaozz
2 days ago

Replying to hellofromTonya:

@azaozz Requests 2.0.0 is fully backwards-compatible...
....
not detecting and handling files in case-insensitive filesystems means Core also could never change the case of an existing file.

Correct. Changing a file's name is a back-compat issue and should never happen unless completely unavoidable. That may be fixed in core in 6.0 but will remain a problem "forever" in other places :(

So my suggestion is to revert these file name changes and release a new version of Requests before updating it in core.

Last edited 45 hours ago by azaozz (previous) (diff)

#32 in reply to: ↑ 31 @SergeyBiryukov
2 days ago

Replying to TobiasBg:

The original merge changeset [52244] mentioned "PHP 8.1 compatibility".
Should relevant changes (if it's not too many?) maybe be backported to the reverted-to Requests version, to achieve (as much as possible) PHP 8.1 compatibility in WP 5.9 (released two months after PHP 8.1, after all)?

Replying to azaozz:

Changing a file's name is a back-compat issue and should never happen unless completely unavoidable. That may be fixed in core in 6.0 but will remain a problem "forever" in other places :(

So my suggestion is to revert these file name changes and release a new version of Requests before updating it in core.

Both of these suggestions seem worth considering to me.

#33 follow-up: @costdev
45 hours ago

@azaozz Also, generally, namespaces should be considered "evil" in open source software and avoided as much as possible. The reason it that they promote "lazy" names that go against the best practice in writing good code (poor self-documenting, confusing names, etc.).

I'm not sure if this is the right place for a discussion on this, but I'd like to hear more about this at some point @azaozz if you're willing. My username's the same on Slack if that works for you. Most open source software has implemented, or is currently implementing, namespacing. I have never seen or heard that namespacing and open source software are, or should be, mutually exclusive or have a minimal relationship.

In terms of going against best practice, that really comes down to a more general issue. You can write poorly self-documenting code with or without namespaces, or you can write well self-documenting code with or without namespaces. There is no consensus in any community that namespaces are always poorly self-documenting. It's a difference of opinion within the development community, but the contemporary standard is to namespace, and that's true across multiple languages.

Best practice for the survival of open source software has always been to respect:

  1. the wisdom that comes with longer-term contribution.
  2. the right of software to exist alongside each other.
  3. that open source software projects can only survive as long as they have contributors, which means that they must adapt to the tried-and-tested practices that fuel contemporary development.

Nearly everything else is down to consensus. Historically, every open source software project that has failed to do these is unfortunately no longer with us - all of us will have experience of a project we've contributed to being closed - It's a sad day when it happens.

Joomla, Drupal, etc. implement namespacing, as do most other similar open source software projects. While we are not those projects, we have a much larger reach and therefore rely even more heavily on a strong contributor base. Namespaces are only one contemporary standard, but dropping them sets a precedent for dropping other contemporary standards and in turn, threatens the size of our contributor base.

In any case, the topic of namespacing within Requests was discussed publicly long ago as @jrf said in an earlier comment. A lot of work has gone into implementing this and while Requests is under the WordPress umbrella, it has a downstream that will also be impacted. Experience tells me that if we start down the path of reverting contemporary standards, we might as well bundle Requests with Core and close the public Requests repo before we watch its usage and contributor base steadily reduce in the coming years, followed by Core some years after.

#34 in reply to: ↑ 33 @azaozz
44 hours ago

Replying to costdev:

I'm not sure if this is the right place...

I'm pretty sure it is not. But as you started it, I'll have to answer your questions :)
I'll be happy to continue this conversation somewhere else at a later time.

In terms of going against best practice, that really comes down to a more general issue. You can write poorly self-documenting code with or without namespaces, or you can write well self-documenting code with or without namespaces.

Right, you can. However having a guarantee that your function or class name will "never" collide with anything is not a good "incentive" to try to come up with a good name. On the opposite, I've been hearing from many people over many years that one of the hardest part of coding is ... picking good descriptive names. My experience over the last 25 or so years is the same :)

the contemporary standard is to namespace, and that's true across multiple languages.

Yeah, namespacing seems to be used more and more (probably because you can easily pick repeating names with it, lol). It doesn't mean it is a good idea in all cases. It seems to be more useful/accepted in a "pure" OOP software (which is most of it), however WP is not OOP by a long shot. Namespacing for "mostly functions" doesn't seem to work that well.

Best practice for the survival of open source software has always been to respect:

Yes, you're right. However thinking this is getting very "philosophical". Whether recommending or not recommending one (mostly insignificant) feature does not affect any of it imho. It is just a recommendation after all.

On the other hand WP will never be able to implement namespacing to most of the PHP code as it will break it pretty badly. So having some parts with namespaces while most of the code is without is somewhat.... not that good? :)

Joomla, Drupal, etc. implement namespacing, as do most other similar open source software projects. While we are not those projects, we have a much larger reach and therefore rely even more heavily on a strong contributor base. Namespaces are only one contemporary standard, but dropping them sets a precedent for dropping other contemporary standards and in turn, threatens the size of our contributor base.

Not talking about dropping, but pretty much all of the current WP PHP code cannot be namespaced (technically not viable). On top of that namespaces do not promote better names. There can be requirements to "ignore" the namespacing and still try to give good, descriptive, unique names to all functions, but eventually these will be avoided or diminish in importance because that's one of the main features of namespacing: to let you get avay with repeating names :)

In any case, the topic of namespacing within Requests was discussed publicly...

That may be but it seems problematic to develop a library as part of WordPress and not take under consideration the established best practices and requirements for WordPress, whether that was discussed somewhere or not. Frankly this is the first time I'm hearing about it and I'm pretty sure it's the same for many other committers.

So in a hindsight: if namespaces cannot be added to WordPress is it a good idea to add them to a WordPress library? Perhaps it is but it would need (at least) a good standard of what function and class names are acceptable and what are not. And all of the community should be aware of it.

#35 follow-up: @jrf
44 hours ago

For everyone here now debatting the issue, while not having read the complete thread or the changelog, let me make something very clear:

The namespacing in Requests 2.0.0 and the file renames are NOT a BC-break.

Let me repeat that: The namespacing in Requests 2.0.0 and the file renames are NOT a BC-break.

The BC-break will come in Requests 4.0.0 a few years down the line.

Yes, the code in Requests is now namespaced, but there is a full backward-compatibility layer in place and using any of the old class names (like if plugins use Requests directly) will still work without problems.

When an old class name is requested, a deprecation notice is thrown. but for the lifetime of Requests 2.0.0, that deprecation notice can be silenced by setting a REQUESTS_SILENCE_PSR0_DEPRECATIONS constant. This has been implemented like this specifically with WP in mind, so plugins which use Requests directly and needing to support multiple WP versions can do so without issue.

Yes, there have been some file renames, but again, this is not a BC-break.

From the very beginning, Requests has had two "entry-points" for loading the files:

  1. Autoloading via a Composer generated autoloader.
  2. Autoloading via a custom autoloader which is included with the Requests code.

The file renames, including the case-only renames, are handled 100% correctly by the both autoloaders.

So where is the BC-break in this whole story ? Well, registering the custom autoloader has changed in Requests 2.0.0 and that change is effectively a two line change and was already included in the patch and will not affect plugins.

So what is this BC-break I mention above in Requests 4.0.0 ? Well, that is the version in which it is the intention to remove the BC-layer.


Now back to the problem at hand: what's breaking here is not Requests 2.0.0. What's breaking is that the WP upgrader process is trying to use a mix of files from Requests 1.x and 2.x during the upgrade process and isn't equipped to handle case-only file renames.

Am I surprised that that breaks things ? No, as that's a very clear bug and code smell. But it's a bug in the upgrader, not in Requests.

Last edited 44 hours ago by jrf (previous) (diff)

@azaozz
44 hours ago

Advantages of namespacing as per the PHP manual. Frankly I see both as being a disadvantages in some cases. Overriding internal funstion and class names would be super confusing for anybody trying to read the code. "Aliasing" long, descriptive names to short non-descriptive is... a bad thing, isn't it? :)

#36 in reply to: ↑ 35 @azaozz
44 hours ago

Replying to jrf:

The namespacing in Requests 2.0.0 and the file renames are NOT a BC-break.

Yeah, I know. Sorry about it. Best to move the conversation to another place, perhaps at https://make.wordpress.org/core/.

Now back to the problem at hand: what's breaking here is not Requests 2.0.0. What's breaking is that the WP upgrader process is trying to use a mix of files from Requests 1.x and 2.x during the upgrade process and isn't equipped to handle case-only file renames.

Am I surprised that that breaks things ? No, as that's a very clear bug and code smell. But it's a bug in the upgrader, not in Requests.

I understand that, but on the other hand files in WordPress should never be renamed unless absolutely unavoidable. If this triggers a bug in WP is there a 100% guarantee it will never trigger a bug somewhere else?

As this is a WordPress (sub) project, I'd think it should follow the established rules/best practices. So the questing is: is this completely unavoidable? If not, I stand by my recommendation to revert and release a new version before updating it in WP.

Last edited 44 hours ago by azaozz (previous) (diff)

#37 in reply to: ↑ 26 ; follow-up: @schlessera
43 hours ago

Replying to azaozz:

If it is under the "WordPress umbrella" it should follow the WordPress best practices, i.e. backwards compatibility is a number one priority. Frankly it is unthinkable that a library developed as part of WordPress would not only break back-compat, but will cause fatal errors when used in core...

If there is such "code style" requirement in that repository it should be removed as incompatible with WordPress. Also, generally, namespaces should be considered "evil" in open source software and avoided as much as possible. The reason it that they promote "lazy" names that go against the best practice in writing good code (poor self-documenting, confusing names, etc.).

Seems a possible/good way forward would be to release Requests 3.0 that will be incompatible with 2.0 but fully compatible with 1.0, then consider it for adding to the next WordPress release. The alternative would be to move the Requests repository out of WordPress and continue developing it as a "true" third-party library that is, unfortunately, incompatible with WordPress.

I will not bother to discuss the benefits of namespacing here, but I just want to state that I feel the above is painfully disrepectful of the many months of hard work that were invested into the Requests library to ensure it keeps working as a secure HTTP abstraction layer for the WordPress project.

Backwards-compatibility was the number one priority, and I can only guess now how @jrf must have felt reading the above, when she literally spent nights and weekends to ensure everything stays compatible.

I'm happy to discuss ways of fixing the upgrader process and will help test it and support it with WP-CLI. But as far as I am concerned, there will not be a Requests v3 to undo the work we did to make that library maintainable again.

#38 follow-up: @hellofromTonya
43 hours ago

Somehow this ticket became a software principles discussion.

Quoting @azaozz:

namespaces should be considered "evil" in open source software and avoided as much as possible

I've had couple of contributors reach out to me privately in slack asking why this statement was made. I'll share my comments to provide balance to opinions already shared.

Namespacing:

  • is common practice across many programming languages including JavaScript, PHP, Python, C#, etc.
  • is common practice in open source projects including PHPMailer (already embedded in Core), WooCommerce, WP-CLI, Drupal, Joomla, Laravel, Symfony, BigCommerce, Composer, PHPUnit, etc.
  • works well for functions, classes, traits, interfaces, etc.

In my opinion:

  • It's not evil.
  • It does not promote "lazy" or bad naming practices.
  • Yes, naming stuff is hard. It's always been hard and I've seen folks struggle to name things throughout my 35 years in engineering.

Quoting @azaozz:

On the other hand WP will never be able to implement namespacing to most of the PHP code as it will break it pretty badly.

I want to point out that actually WordPress could add namespaces if a BC-layer is also created (could follow Requests 2.0.0 example).

WordPress is not limited or held back from adding namespaces. Actually, they already exist in Core with the external SimplePie library.

For Core's existing code, decisions would need to be made about where it makes sense to use it. For new code, it could be added if the coding standards allowed it.

If there's interest in introducing namespacing into Core beyond external libraries like SimplePie and Requests, then that discussion deserves its own ticket.

Last edited 32 hours ago by hellofromTonya (previous) (diff)

#39 in reply to: ↑ 38 @azaozz
43 hours ago

Replying to hellofromTonya:

Somehow this ticket became a software principles discussion.

Yeah, sorry, I shouldn't have responded to @costdev's comment, my bad.

I've had couple of contributors reach out to me privately in slack asking...

Well it was meant as a joke, mostly, hence the quotes. Interesting though why people have been reaching out to you and not to me to explain what I meant. Wouldn't it be better to get it "straight from the horse's mouth"? :)

Namespacing:
...
I want to point out that actually WordPress could add namespaces if a BC-layer is also created.

Lots of unneeded complexity for a seemingly small, insignificant improvement (that can actually be used to lower the code quality in the long run)? But lets take it to https://make.wordpress.org/core/, can we, please?

#40 in reply to: ↑ 37 ; follow-up: @azaozz
42 hours ago

Replying to schlessera:

the above is painfully disrepectful of the many months of hard work

I'm sorry you feel that way but these things (small errors that "upset the applecart") happen sometimes to everybody, me including.

So basically you're saying that changing the case in the names is an essential, irreplaceable part of Requests 2.0 and this cannot be changed again without throwing all the other work out? In this case, as I pointed out above, lets consider it an exception to the WordPress best practices rule for never changing file names and be done with it?

#41 in reply to: ↑ 40 @schlessera
36 hours ago

Replying to azaozz:

I'm sorry you feel that way but these things (small errors that "upset the applecart") happen sometimes to everybody, me including.

Oh, I'm always up for "upsetting the apple cart". But your words seemed to imply that we just changed Requests on a whim and put no thoughts into it whatsoever, and I certainly hope that neither @jrf nor I are giving the impression that that is how we operate.

So basically you're saying that changing the case in the names is an essential, irreplaceable part of Requests 2.0 and this cannot be changed again without throwing all the other work out?

No, not at all. What I'm saying is that the current maintainers of Requests will not undo their work and change back the maintenance improvements they've made, as I'd consider this "making Requests worse to avoid having to make WordPress better".

As rigid as WordPress may be, at the very least its upgrader process (!) needs to account for change - the one constant that is always true for software projects. If it does not, it is failing its purpose.

Note: See TracTickets for help on using tickets.