WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#36926 closed enhancement (fixed)

Autoloading: Introduce compatibility shim for spl_autoload_register

Reported by: rmccue Owned by: rmccue
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch commit
Focuses: Cc:

Description

Split from #36335.

To ensure support for multiple autoloaders when SPL isn't available (5.2 with SPL disabled; 5.3+ require SPL), we should polyfill the autoloader registration code.

Attachments (4)

autoload-compat.php (1.8 KB) - added by rmccue 3 years ago.
SPL autoload compatibility shim/polyfill
36926.diff (2.3 KB) - added by rmccue 3 years ago.
Initial patch to add to compat.php
36926.2.diff (4.0 KB) - added by rmccue 3 years ago.
Remove spl_autoload_register check
36926.3.diff (4.1 KB) - added by rmccue 3 years ago.
Add is_callable check

Download all attachments as: .zip

Change History (18)

@rmccue
3 years ago

SPL autoload compatibility shim/polyfill

@rmccue
3 years ago

Initial patch to add to compat.php

@rmccue
3 years ago

Remove spl_autoload_register check

#1 @rmccue
3 years ago

  • Keywords has-patch 2nd-opinion added
  • Milestone changed from Awaiting Review to 4.6

Added an actual patch for core; renamed from $spl_autoloaders to $_wp_spl_autoloaders to indicate that a) it's part of a shim and shouldn't be used, and b) to avoid any potential variable conflicts. While we could introduce a class with protected/private vars to actually protect it, that's too much overhead for just a shim, especially one that's only loaded on a small number of installs.

This doesn't really introduce any new stuff into core, apart from the compatibility shim for 5.2, so it should be pretty uncontroversial. Core does actually use this with the introduction of Requests, plus we can remove the check in the SimplePie loader (see 36926.2.diff. Based on this, I'd like to get it into core this cycle, regardless of progress on #36335 (although I hope we can get that in too!).

Version 0, edited 3 years ago by rmccue (next)

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


3 years ago

#3 follow-ups: @tfrommen
3 years ago

Hey Ryan,

since you copied stuff from the Autoloader ticket, I do that as well. ;)

We actually spoke about it, but it hasn't made it into your code yet.

Improve __autoload()
$spl_autoloaders is global, so anyone can put into it whatever they like. So we should check for is_callable for each autoloader function inside __autoload(), and continue if it isn't.

Improve spl_autoload_functions()
Since $spl_autoloaders is, again, a global, and since we don't want to argue with SPL, we should make sure we're returning an array.

<?php

function spl_autoload_functions() {
        global $spl_autoloaders;
        
        return is_array( $spl_autoloaders ) ? $spl_autoloaders : array();
}

#4 in reply to: ↑ 3 @dd32
3 years ago

@rmccue 36926.2.diff seems pretty sane to me.
The obvious breakage is when something else is defining __autoload(), but I think that's something we're just going to have to expect - there's no way to polyfill it in those cases.

Replying to tfrommen:

Improve __autoload()
$spl_autoloaders is global, so anyone can put into it whatever they like. So we should check for is_callable for each autoloader function inside __autoload(), and continue if it isn't.

Improve spl_autoload_functions()
Since $spl_autoloaders is, again, a global, and since we don't want to argue with SPL, we should make sure we're returning an array.

Both of these seem like needless checks in this case, if outside code modifies the global their warranty is void, just as if they modify the global $wp_filters arrays, or almost any other global in WordPress for that matter.

#5 in reply to: ↑ 3 ; follow-up: @rmccue
3 years ago

Replying to tfrommen:

We actually spoke about it, but it hasn't made it into your code yet.

Oops, sorry about that! Forgot to incorporate it here.

While I agree with @dd32 that your warranty is void if you do that, I think it makes sense to be defensive here. As it turns out, PHP throws an exception if the callback can't be found, so we should match that. The array check is also cheap, so makes sense to do that.

#6 in reply to: ↑ 5 ; follow-up: @dd32
3 years ago

Replying to rmccue:

As it turns out, PHP throws an exception if the callback can't be found, so we should match that.

You already support that :) PHP doesn't throw an exception when it's calling the autoloaders (https://3v4l.org/Fj7I1), only when you first try to register it.

#7 in reply to: ↑ 6 @rmccue
3 years ago

Replying to dd32:

You already support that :) PHP doesn't throw an exception when it's calling the autoloaders (https://3v4l.org/Fj7I1), only when you first try to register it.

Oh you're totally right, I misread the exception! However, the current patch will throw a warning from call_user_func, which doesn't match the PHP implementation.

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


3 years ago

@rmccue
3 years ago

Add is_callable check

#9 @rmccue
3 years ago

  • Keywords 2nd-opinion removed

Added an is_callable check to __autoload to ensure we don't generate excess warnings. I didn't add the is_array check, because a) if you directly change that, you've voided your warranty, and b) someone might want to replace it with an array-like object or something, and I don't think there's a need to be super strict.

While this isn't absolutely optimal (I'd be concerned about performance with is_callable usually), this is only a shim for 5.2, so I don't think performance matters. If you need more performant code, use 5.6+ (ideally, PHP 7).

I think this is ready for commit.

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


3 years ago

#11 @dd32
3 years ago

  • Keywords commit added

Looks good to me, commit it.

#12 @rmccue
3 years ago

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

In 37636:

Autoload: Introduce shim for SPL autoloading.

For PHP 5.2, SPL can be disabled. As SPL provides the support for multiple autoloaders, this needs to be shimmed if not available.

Fixes #36926.

#13 @rmccue
3 years ago

In 37637:

Autoload: Add missed @since tags to SPL shim.

See #36926.

This ticket was mentioned in Slack in #core-restapi by jorbin. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.