-
Notifications
You must be signed in to change notification settings - Fork 493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Requests 2.0.0: introduce namespaces #519
Conversation
Note: there is still something weird going on with the WP Core tests, the PHPUnit tests pass just fine, but there are a couple of E2E tests not passing. Honestly no clue what's going on there. |
I think I have a clue ;-) The "Install WordPress" step fails due to a fatal "Class already declared" error in the interaction between WP Core and WP CLI: https://github.com/WordPress/wordpress-develop/runs/3457524420?check_suite_focus=true#step:12:188 I suspect that may be the cause. I have added two extra commits which should prevent those kind of errors in common situations, though they don't yet solve it for the WP GHA workflow. |
035b835
to
f3b935f
Compare
Found a few more class references in documentation which needed updating - fixed up in the "Requests: update all references to the class" commit. No other changes. |
f3b935f
to
b6ab51a
Compare
I have rebased the PR after today's merges and made the following changes as discussed in today's call:
|
The PR testing the WordPress integration of Requests with this PR has been updated with the latest changes. Edit: As expected, this change does not make a difference: https://github.com/WordPress/wordpress-develop/runs/3505566455?check_suite_focus=true#step:12:188 |
* Allow for files in the `src` directory to be scanned based on the same rules as before. * Allow for the `WpOrg\Requests` namespace prefix.
Includes: * Adding a safeguard against the autoloader being registered multiple times. * Registering the autoloader via prepending it to the autoload queue to prevent potential conflicts with a Requests 1.x autoloader. * Deprecating the old methods in the original `Requests` class, while still safeguarding that they will continue to work for now. * Minor adjustments to the code to allow for the change in directory.
b6ab51a
to
3a33546
Compare
@schlessera did some additional testing with a project which has the Requests library as a dependency and discovered that the issues with the WP integration described above, are due to the Composer PSR-4 implementation having a PSR-0 falll-back, which was confusing everything due to the old PSR-0 implementation and the new PSR-4 implementation both using After some discussion we have decided to rename the new namespace from To that end, I've:
Both the CI build for this PR, as well as the test build for an upgrade of the Requests library included in WordPress - see WordPress/wordpress-develop#1624 - pass after these additional changes, so as far as I'm concerned, this PR is ready for a final review and if nothing new is found, merging. |
1134cd5
to
9e6920f
Compare
FYI: last push was just some minor changes to improve the text formatting for the markdown in the |
9e6920f
to
14afbfe
Compare
Made a minor tweak to the "Upgrade guide" commit to add the Previous/Next links at the bottom of the page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason why the classes are being referenced as FQCNs in the docblocks, even though they are already being imported?
There are unused imports in multiple classes, some of them due to the above FQCNs being used, some generally because all referenced classes are in the same subnamespace.
It also bothers me that the But we can change that in a separate PR, as it might cause more discussion. |
This commit: * Adds a PSR-4 directive for the `src` directory to the `composer.json` file to support autoloading of the new classes when users use the `vendor/autoload.php` file to handle the class loading. * Adds support for PSR-4 based class loading to the `WpOrg\Requests\Autoload` class for users who use the custom autoload functionality provided by Requests. Notes: * The autoloader will now always return a boolean value, which improves compatibility with autoloader chaining. * The old `Requests` class is special cased. While it is probably rare for this class to be requested for autoloading via the new autoloader, it should still be handled correctly if it is. As the class in the new structure will be split into two classes: `WpOrg\Requests\Requests` and this `WpOrg\Requests\Autoload` class, the file which will be loaded will be the old `Requests` class which will extend the `WpOrg\Requests\Requests` class and will use the `WpOrg\Requests\Autoload` class for the autoloading, maintaining the old functionality without BC break. * When PSR-0 class names which are no longer in use are requested, the new PSR-4 class name will be aliased to the PSR-0 class name for backward compatibility. - The first time during a "page load" such an alias is created, a deprecation notice will be thrown. - Only one deprecation notice per "page load" will be thrown. - Users of this library, which need to support both Requests 1.x as well as 2.x, can declare a `REQUESTS_SILENCE_PSR0_DEPRECATIONS` constant (before the autoloader is invoked for the first time) to silence these deprecation notices. This constant will be supported throughout the lifetime of Requests v 2.x and will be removed in Requests 3.0.0. * As quite a few directory names and class names will have changes in the letter case they use between PSR-0 and PSR-4 and there have also been changes in the class names for the HTTP status exceptions, I'm choosing to implement the matching of the PSR-0 classes to the PSR-4 class names via a case-insensitive lookup table. While the logic behind the renames is transparent and can be encoded in a function, using a lookup table provides optimal speed, especially as the list of classes for which aliasing is provided is limited and will not change.
... to use the new `WpOrg\Requests\Autoload` class instead of the old `Requests` class and use the `Autoload::register()` method instead of the `Requests::register_autoloader()` method.
... while the classes are being namespaced.
Includes: * Moving the namespaced version of the class to the `src` directory. * [Custom autoload] Adding the class to the `$deprecated_classes` array in the `WpOrg\Requests\Autoload` class. * [Composer autoload] Adding the class to the `Deprecated.php` file. * Updating all references to the class.
Includes: * Using CamelCaps for the class name, as well as the test class. * Moving the namespaced version of the class to the `src` directory. * [Custom autoload] Adding the class to the `$deprecated_classes` array in the `WpOrg\Requests\Autoload` class. * [Composer autoload] Adding the class to the `Deprecated.php` file. * Updating all references to the class.
Includes: * Moving the namespaced version of the class to the `src` directory. * [Custom autoload] Adding the class to the `$deprecated_classes` array in the `WpOrg\Requests\Autoload` class. * [Composer autoload] Adding the class to the `Deprecated.php` file. * Updating all references to the class.
Includes: * Using CamelCaps for the class name, as well as the test class. * Moving the namespaced version of the class to the `src` directory. * [Custom autoload] Adding the class to the `$deprecated_classes` array in the `WpOrg\Requests\Autoload` class. * [Composer autoload] Adding the class to the `Deprecated.php` file. * Updating all references to the class. * Minor PHPCS tweak.
Includes: * Using CamelCaps for the class name. * Moving the namespaced version of the class to the `src` directory. * [Custom autoload] Adding the class to the `$deprecated_classes` array in the `WpOrg\Requests\Autoload` class. * [Composer autoload] Adding the class to the `Deprecated.php` file. * Updating all references to the class. * Minor PHPCS config tweak.
Includes: * Moving the namespaced version of the class to the `src` directory. * [Custom autoload] Adding the class to the `$deprecated_classes` array in the `WpOrg\Requests\Autoload` class. * [Composer autoload] Adding the class to the `Deprecated.php` file. * Updating all references to the class.
Includes: * Moving the namespaced version of the class to the `src` directory. * [Custom autoload] Adding the class to the `$deprecated_classes` array in the `WpOrg\Requests\Autoload` class. * [Composer autoload] Adding the class to the `Deprecated.php` file. * Updating all references to the class.
Includes: * Using CamelCaps for the class name. * Moving the namespaced version of the class to the `src` directory. * [Custom autoload] Adding the class to the `$deprecated_classes` array in the `WpOrg\Requests\Autoload` class. * [Composer autoload] Adding the class to the `Deprecated.php` file. * Updating all references to the class.
Includes: * Moving the namespaced version of the class to the `src` directory. * Updating all references to the class. * Adding a missing `@since` tag. Note: as this class has only been introduced in Requests 2.0.0, the original class name has not been added to the class alias list or the deprecated classes file.
Includes: * Moving the namespaced version of the class to the `src` directory. * [Custom autoload] Adding the class to the `$deprecated_classes` array in the `WpOrg\Requests\Autoload` class. * [Composer autoload] Adding the class to the `Deprecated.php` file. * Updating all references to the class.
Includes: * Using CamelCaps for the class name. * Moving the namespaced version of the class to the `src` directory. * [Custom autoload] Adding the class to the `$deprecated_classes` array in the `WpOrg\Requests\Autoload` class. * [Composer autoload] Adding the class to the `Deprecated.php` file. * Updating all references to the class. * Minor PHPCS config tweak.
Includes: * Prefix the class names with `Status` as purely numeric class names are not allowed in PHP. Note: while `Unknown` would have been allowed, I've elected to prefix this with `Status` as well for consistency. * Moving the namespaced version of the classes to the `src` directory. * [Custom autoload] Adding the classes to the `$deprecated_classes` array in the `WpOrg\Requests\Autoload` class. * [Composer autoload] Adding the classes to the `Deprecated.php` file. * Updating all references to the classes.
... as Requests now no longer contains any PSR-0 classes. PSR-0 classes are still supported in the backward-compatibility layer, which means that: * When using Composer for the autoloading, the PSR-0 classes will be made available via a classmap. * When using the Requests native custom autoloading, the PSR-0 classes will be aliased to their PSR-4 equivalent.
... as all classes are now properly namespaced. Also: switch to the new `WpOrg\Requests\Autoload` class for the autoloading of the files when using a PHPUnit PHAR file to run the tests.
... to point to the `src` instead of the `library` directory.
Note: the Symfony Class Loader package is deprecated and was removed from Symfony in version 4.0, so we may want to replace this example with one using a still maintained external library.
…on in an "exists" check This fixes a potential fatal "class already exists" error when during one "page load": * Two projects both using Requests are used. * Neither of these projects uses Requests with a custom namespace/prefix as can be done via PHP_Scoper. * The first of these projects loaded is still using Requests 1.x and is using Composer autoload. * The second project is using Requests 2.x and is also using Composer autoload, In that case, depending on the order of the registered autoloaders, the `Deprecated.php` file _may_ be called while _some_ Requests 1.x have already been autoloaded, but not all. Bulk declaring the deprecated classes will then lead to a "Fatal error: Cannot declare interface/class Requests_..., because the name is already in use" error. Using `class_exists()` wrappers will prevent this. In addition to this, the conditions also check for the existance of the "new" classes to prevent potential fatal errors when only a selection of the classes from this library are used/available within an external project.
... when the old PSR-0 classes are being autoloaded.
... from `Requests\Tests` to `WpOrg\Requests\Tests` for consistency.
14afbfe
to
84efc00
Compare
... to prevent there ever being more than one deprecation notice in one page load.
Let's change that in the "magic numbers" PRs - I've got a commit lined up already to change something regarding that anyhow. |
Recap of what was discussed about this:
For now for consistency and to make sure that the initial docs run with the new doc generator will work without problems. At a later point, I suggest we define how we want to deal with docblocks, same namespace, functions, constants etc consistently in the codebase for the future and we can add CS rules to enforce it. I didn't want to muddy the waters in this PR mixing those changes into this as well.
Same namespace unused |
TL;DR:
This PR:
src
directory.library
directory and the remaining files in it are deprecated and marked as such.CamelCaps
.This BC-layer:
REQUESTS_SILENCE_PSR0_DEPRECATIONS
constant with the valuetrue
.WpOrg\Requests\Autoload
class and the whole of thelibrary
folder) will be removed in Requests 4.0.0.docs
folder with an upgrade guide to Requests 2.0.0 based on the changes made in this PR.Related to #460
QA
As part of the QA for this PR, a draft upgrade of the Requests library for WordPress has been executed and pulled and the unit tests pass: WordPress/wordpress-develop#1624
(there is still an issue with the E2E tests which is related to the interaction with WPCLI being investigated)To do
Commit Details
PHPCS: few tweaks
... to allow for files in the
src
directory to be scanned based on the same rules as before.Requests: move autoload functionality to separate class
Includes:
Requests
class, while still safeguarding that they will continue to work for now.Update autoloading to allow for PSR-4 class names
This commit:
src
directory to thecomposer.json
file to support autoloading of the new classes when users use thevendor/autoload.php
file to handle the class loading.WpOrg\Requests\Autoload
class for users who use the custom autoload functionality provided by Requests.Notes:
Requests
class is special cased.While it is probably rare for this class to be requested for autoloading via the new autoloader, it should still be handled correctly if it is.
As the class in the new structure will be split into two classes:
WpOrg\Requests\Requests
and thisWpOrg\Requests\Autoload
class, the file which will be loaded will be the oldRequests
class which will extend theWpOrg\Requests\Requests
class and will use theWpOrg\Requests\Autoload
class for the autoloading, maintaining the old functionality without BC break.REQUESTS_SILENCE_PSR0_DEPRECATIONS
constant (before the autoloader is invoked for the first time) to silence these deprecation notices.This constant will be supported throughout the lifetime of Requests v 2.x and will be removed in Requests 3.0.0.
While the logic behind the renames is transparent and can be encoded in a function, using a lookup table provides optimal speed, especially as the list of classes for which aliasing is provided is limited and will not change.
Requests: update all code samples using the custom autoloader
.. to use the new
WpOrg\Requests\Autoload
class instead of the oldRequests
class with theregister_autoloader()
method.Tests: temporarily silence deprecation warnings
... while the classes are being namespaced.
Requests: move the real functionality to a namespaced Requests class
Move the real
Requests
class - with the exception of the autoload functionality - to thesrc
directory and namespace it.Includes updating the reference to this class in the release checklist.
Requests: update all references to the class
use
statements referencing the class.use
import statements to all non-namespaced classes using theRequests
class.Requests: deprecate the remaining class
Deprecate all functionality which remains in the old non-namespaced
Requests
class and add a deprecation notice for when people would stillrequire
the class directly.Includes:
WpOrg\Requests\Autoload
file from throwing a duplicate deprecation notice during a page load, when theRequests
class was loaded as well as a class which is being aliased.Move certificates file to src directory
Requests_Auth
/Requests_Hooker
/Requests_Proxy
/Requests_Transport
: change to namespaced interfacesIncludes:
src
directory.$deprecated_classes
array in theWpOrg\Requests\Autoload
class.Deprecated.php
file.Same as the custom autoloader when encountering deprecated interfaces/classes, loading this file will also trigger a deprecation notice.
And again, same as the custom autoloader, the deprecation notice can be silenced by setting a
REQUESTS_SILENCE_PSR0_DEPRECATIONS
constant.Deprecated.php
file to the Composerautoload
directive to be indexed to a classmap.Deprecated.php
file to contain multiple class declarations.Requests_[...]
(all classes): change to namespaced classIncludes:
src
directory.$deprecated_classes
array in theWpOrg\Requests\Autoload
class.Deprecated.php
file.Note 1: As the
Requests_Exception_InvalidArgument
class has only been introduced in Requests 2.0.0, the original class name has not been added to the class alias list or the deprecated classes file.Note 2: The class names of the
Requests_Exception_HTTP_*
classes, have been Prefixed withStatus
as purely numeric class names are not allowed in PHP.Note: while
Unknown
would have been allowed, I've elected to prefix this withStatus
as well for consistency.Remove support for PSR-0 autoloading
... as Requests now no longer contains any PSR-0 classes.
PSR-0 classes are still supported in the backward-compatibility layer, which means that:
Tests: undo the temporarily silencing of deprecation warnings
... as all classes are now properly namespaced.
Also: switch to the new
WpOrg\Requests\Autoload
class for the autoloading of the files when using a PHPUnit PHAR file to run the tests.Document the library directory
Documentation: add page about upgrading to Requests 2.0
Code coverage: update configuration
... to point to the
src
instead of thelibrary
directory.README: update external class loader example code
Note: the Symfony Class Loader package is deprecated and was removed from Symfony in version 4.0, so we may want to replace this example with one using a still maintained external library.
Added three more commits after the initial PR was pulled:
🆕 Deprecated classes classmap file: wrap each class/interface declaration in an "exists" check
This fixes a potential fatal "class already exists" error when during one "page load":
In that case, depending on the order of the registered autoloaders, the
Deprecated.php
file may be called while some Requests 1.x have already been autoloaded, but not all.Bulk declaring the deprecated classes will then lead to a "Fatal error: Cannot declare interface/class Requests_..., because the name is already in use" error.
Using
class_exists()
wrappers will prevent this.🆕
WpOrg\Requests\Autoload: prepend the new autoloader🆕 Tests: add minimal test to verify that deprecation notices are thrown
... when the old PSR-0 classes are being autoloaded.
🆕 🆕 Tests: rename the namespace
... from
Requests\Tests
toWpOrg\Requests\Tests
for consistency.