Make WordPress Core

Opened 17 months ago

Closed 16 months ago

Last modified 16 months ago

#57341 closed defect (bug) (fixed)

`Error: Class "Requests" not found` in trunk

Reported by: bjorsch's profile bjorsch Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.2 Priority: normal
Severity: normal Version: 6.2
Component: HTTP API Keywords: has-testing-info has-patch has-unit-tests commit
Focuses: Cc:

Description

With the merge of the updated Requests library (thank you!), there's one thing that seems to have remained broken. Use of the old Requests class does not work since that class is not loaded by the new library's autoloader like everything else is.

The problem seems to be in wp-includes/Requests/src/Autoload.php lines 141–143:

if ($class_lower === 'requests') {
        // Reference to the original PSR-0 Requests class.
        $file = dirname(__DIR__) . '/library/Requests.php';

It tries, but it uses the wrong file path because the necessary file is in a different place in WordPress versus the Requests library. If that last line is changed to this

        $file = dirname(dirname(__DIR__)) . '/class-requests.php';

then it works.

I have no idea whether you'll want to fix this in WordPress core or in the Requests library somehow to sync back into core, so I'll leave it to you to produce the patch.

Change History (17)

#1 @costdev
17 months ago

  • Keywords needs-testing-info added

Hi @bjorsch, thanks for opening this ticket!

Can you provide some instructions for a quick reproduction of the error for testing the issue and a potential final patch?

Pinging @hellofromTonya, @jrf and @schlessera for awareness.

#2 @bjorsch
17 months ago

You can add this as a mu-plugin to reproduce it.

<?php

function test_57341() {
        ?>
<div style="border:10px solid red"><?php echo Requests::GET; ?></div>
        <?php
}
add_action( 'wp_head', 'test_57341' );

On 6.1.1 it produces a box with the word "GET" at the top of the page, while with trunk it produces a fatal error.

#3 follow-up: @jrf
17 months ago

Thanks for reporting this @bjorsch.

As far as I can see, there are two parts to this issue, none of which are for Requests to solve.

Primarily, this is an issue with your code, which needs to be updated for Requests 2.0.0 (and yes, this still needs a dev-note).

To upgrade the code to Requests 2.0.0, it should be changed to:

<?php

function test_57341() {
        ?>
<div style="border:10px solid red"><?php echo WpOrg\Requests\Requests::GET; ?></div>
        <?php
}
add_action( 'wp_head', 'test_57341' );

However, generally speaking, code referring to the "old" Requests classes should still continue to work. The BC-layer allows for that.

Which brings us to the second part:

In WP Core, the "old" Requests class is in wp-includes/class-requests.php, while in Requests itself, the class is in library/Requests.php.

No surprise then that the Requests Autoloader cannot locate the class...

To solve this, I'll leave two suggestions:

  1. Edit line 143 in the wp-includes/Requests/src/Autoload.php file to point to the wp-includes/class-requests.php file instead. Doing this will make future upgrades of Requests more difficult.
  2. Add a require ABSPATH . WPINC . '/class-requests.php to the bit at the top of the wp-includes/class-wp-http.php file which also requires and registers the Requests autoloader. Note: this will trigger a deprecation notice for loading a deprecated class, but that is intentional and correct.

To be honest, I'm not even sure this _should_ be solved in Core. Plugins which want to continue to use the old class names can do so (and can manually include the wp-includes/class-requests.php file), but should expect deprecation notices (which can be silenced).

Alternatively, they should update their code to embrace Requests 2.0.0.

This is the same as what previously had to happen for plugins which referred directly to the external PHPMailer dependency, they have had to update their code as well as that class became namespaced a while back. This is no different.

#4 @jrf
17 months ago

Oh... just thinking.. what may be an elegant way to solve this, preventing the fatal, but still putting the onus of upgrading their code on plugin developers:

Add a wp-includes/Requests/library/Requests.php file with the only content in that file being include_once ABSPATH . WPINC . '/class-requests.php';.

Last edited 17 months ago by jrf (previous) (diff)

This ticket was mentioned in PR #3776 on WordPress/wordpress-develop by @costdev.


17 months ago
#5

  • Keywords has-patch has-unit-tests added

This PR:

Adds the wp-includes/Requests/library/Requests.php file, which includes the old Requests class in wp-includes/class-requests.php for plugins and themes that still use it.

This will trigger a deprecation notice, which is expected and intentional, to notify extenders to update their plugin or theme at their earliest convenience.

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

#6 in reply to: ↑ 3 ; follow-up: @bjorsch
17 months ago

  • Keywords has-patch has-unit-tests removed

Replying to jrf:

Primarily, this is an issue with your code, which needs to be updated for Requests 2.0.0 (and yes, this still needs a dev-note).

Unfortunately we can't easily do that while continuing to support WordPress 6.1. It was my understanding that this is what the BC layer is for. Sure, we could reinvent the BC layer in our own plugin, but why should we when core already has one?

(For our plugins, we generally support the current WP release and one previous, so we'd drop 6.1 when 6.3 is released. I can't say what any other plugins might do, of course.)

In WP Core, the "old" Requests class is in wp-includes/class-requests.php, while in Requests itself, the class is in library/Requests.php.

That matches what I found. 👍

To solve this, I'll leave two suggestions:

  1. Edit line 143 in the wp-includes/Requests/src/Autoload.php file to point to the wp-includes/class-requests.php file instead. Doing this will make future upgrades of Requests more difficult.
  2. Add a require ABSPATH . WPINC . '/class-requests.php to the bit at the top of the wp-includes/class-wp-http.php file which also requires and registers the Requests autoloader. Note: this will trigger a deprecation notice for loading a deprecated class, but that is intentional and correct.

I thought of a third, a little after I originally filed this bug.

  1. Add a file at wp-includes/Requests/library/Requests.php where the existing Autoload code looks for it, which does the require ABSPATH . WPINC . '/class-requests.php' bit to load the file from where WordPress has it.

(EDIT: Ah, I see that was already suggested in later replies. That's what I get for replying before reading everything.)

To be honest, I'm not even sure this _should_ be solved in Core. Plugins which want to continue to use the old class names can do so (and can manually include the wp-includes/class-requests.php file), but should expect deprecation notices (which can be silenced).

I think it should. The deprecation notices are expected when using the BC layer, but a fatal is not.

Last edited 17 months ago by bjorsch (previous) (diff)

#7 @bjorsch
17 months ago

  • Keywords has-patch has-unit-tests added

#8 in reply to: ↑ 6 @jrf
17 months ago

Replying to bjorsch:

Replying to jrf:

To be honest, I'm not even sure this _should_ be solved in Core. Plugins which want to continue to use the old class names can do so (and can manually include the wp-includes/class-requests.php file), but should expect deprecation notices (which can be silenced).

I think it should. The deprecation notices are expected when using the BC layer, but a fatal is not.

Correct and I support an implementation of the third option, which we both suggested. (snap :high-five:)

#9 @costdev
17 months ago

  • Keywords has-testing-info added; needs-testing-info removed
  • Milestone changed from Awaiting Review to 6.2

Add a wp-includes/Requests/library/Requests.php file with the only content in that file being include_once ABSPATH . WPINC . '/class-requests.php';

PR 3776 implements this suggestion and includes a PHPUnit test. The PR has two commits: One to show the issue, and one to show the change resolving it.

Last edited 17 months ago by costdev (previous) (diff)

#10 @costdev
17 months ago

  • Keywords commit added

@bjorsch has confirmed on the PR that this change resolves the issue on a testing site.

Adding commit for final review and commit consideration.

#11 @ironprogrammer
17 months ago

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/3776.diff 👍🏻

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 12.6.1
  • Browser: Safari 16.1
  • Server: nginx/1.23.2
  • PHP: 7.4.33
  • WordPress: 6.2-alpha-54642-src
  • Active Plugins:
    • trac-57341 (mu-plugin described in comment:2)

Actual Results

  • ✅ Call to non-namespaced Requests::GET no longer fatals, and produces the following deprecation notice:
    Deprecated: The PSR-0 `Requests_...` class names in the Request library are deprecated. Switch to the PSR-4 `WpOrg\Requests\...` class names at your earliest convenience. in .../wp-includes/class-requests.php on line 24
    
  • ✅ Call to namespaced WpOrg\Requests\Requests::GET succeeds and does not produce a deprecation notice.

#12 @mukesh27
17 months ago

Thanks @bjorsch for the ticket and @costdev for the PR.

PR looks good to me. Left some nit-pick feedback on PR.

#13 follow-up: @peterwilsoncc
16 months ago

I'm seeing quite a few plugins that do seem to reference the older file structure directly in require statements, not just the autoloader file.

However wise it was for these plugins to do this can be debated but WordPress should not throw a fatal error on sites including plugins that did so.

There are some false positives in the link (security plugins containing the file hash been the most common) there are also a quite a number of installations that are true positives.

#14 in reply to: ↑ 13 @jrf
16 months ago

Replying to peterwilsoncc:

I'm seeing quite a few plugins that do seem to reference the older file structure directly in require statements, not just the autoloader file.

However wise it was for these plugins to do this can be debated but WordPress should not throw a fatal error on sites including plugins that did so.

There are some false positives in the link (security plugins containing the file hash been the most common) there are also a quite a number of installations that are true positives.

Thanks for your concern @peterwilsoncc.

I have reviewed the complete first page of the results and nearly everything is false positive (either references in comments, which will have no impact, composer.lock files or includes for a completely different package).

It looks like a lot of plugins are using some other package, which also installs in a Requests directory, but is not actually the Requests package we are talking about here.

The only non-false positives are:

  • Real Cookie Banner: GDPR (DSGVO) & ePrivacy Cookie Consent
  • Real Custom Post Order: Create a custom order for your content
  • Real Category Management: Content Management in Category Folders
  • Real Thumbnail Generator: Efficient regeneration of thumbnails in all sizes
  • NFT Maker

These all seem to use the same utility package - devowl-wp/utils - referencing two files from Requests in the WP directory structure, so in reality there is one (1) package doing_it_wrong, though those requires are wrapped in a class_exists() function call and as class_exists() triggers the autoloader, they shouldn't actually be problematic as the requires will never be executed.

Note, the following plugins do reference files from Requests, but NOT the ones in the WP directory structure, but in a vendor directory (or similar) shipped with the plugin. Whether that is wise (unless prefixed) is a whole different debate, but not a concern for this ticket.

  • Duplicator – WordPress Migration Plugin
  • Razorpay for WooCommerce
  • Appointment and Event Booking Calendar for WordPress – Amelia
  • WCFM Marketplace – Best Multivendor Marketplace for WooCommerce
  • Italy Cookie Choices (for EU Cookie Law & Cookie Notice)
  • AliExpress Dropshipping with Ali2Woo Lite
  • Razorpay Quick Payments
  • Knit Pay – Instamojo, Razorpay, PayU, Cashfree, Stripe, Easebuzz, UPI QR, CCAvenue
  • Culqi Integracion
  • WP PHP Console
  • Infogram – Add charts, maps and infographics
  • Razorpay Payment Button Elementor Plugin
  • Razorpay Payment Button Plugin
  • Kata Plus
  • Razorpay for Gravity Forms

#15 @hellofromTonya
16 months ago

  • Owner set to hellofromTonya
  • Status changed from new to reviewing

Self-assigning ownership for review.

#16 @hellofromTonya
16 months ago

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

In 55007:

HTTP API: Adds BC-layer /library/Requests.php file.

When the Requests 2.0.0 autoloader detects the older (deprecated) `Requests` class, it attempts to load its `/library/Requests.php` file. Prior to this commit, this file did not exist in Core. Thus, a fatal error happened.

Why not change Requests Autoloader?
Requests is an external dependency that Core consumes. It is also used by other projects outside of Core. Thus, Core needs a fix to guard itself to prevent a fatal error.

The fix:

  • Adds the missing wp-includes/Requests/library/Requests.php file, which then loads the wp-includes/class-requests.php (which will throw a deprecation notice to alert developers to upgrade).
  • Adds a test.

Follow-up to [54997].

Props bjorsch, costdev, jrf, mukesh27, peterwilsoncc, ironprogrammer, hellofromTonya.
Fixes #57341.

@hellofromTonya commented on PR #3776:


16 months ago
#17

Committed via changeset https://core.trac.wordpress.org/changeset/55007. Thank you everyone for your contributions!

Note: See TracTickets for help on using tickets.