Skip to content
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

Autoload: introduce method get_deprecated_classes(). #681

Merged
merged 2 commits into from
Feb 28, 2022

Conversation

alpipego
Copy link
Contributor

@alpipego alpipego commented Feb 9, 2022

This method will return the array for the class mapping between the
legacy classes and their namespaced 2.x counterparts. The need for this
change came up while using the 2.x version of the library together
with a framework or CMS, that includes a 1.x version of Requests.

In order to prevent the framework from autoloading the 1.x classes,
they must be aliased before accessed. Through exposing the mapping
array, the classes can be aliased before being their implementations
get loaded.

See #659

Pull Request Type

  • I have checked there is no other PR open for the same change.

This is a:

  • Bug fix
  • New feature
  • Code quality improvement

Context

Detailed Description

This adds the method \WpOrg\Requests\Autoload::get_deprecated_classes to access the class mapping from v1 to v2 and load the classes before \WP_Http gets the chance to do so.

Quality assurance

  • This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added unit tests to accompany this PR.
  • The (new/existing) tests cover this PR 100%.
  • I have (manually) tested this code to the best of my abilities.
  • My code follows the style guidelines of this project.

Documentation

For new features:

  • I have added a code example showing how to use this feature to the examples directory.
  • I have added documentation about this feature to the docs directory.
    If the documentation is in a new markdown file, I have added a link to this new file to the Docs folder README.md file.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alpipego Looks like the example file mentioned in #659 (comment) is missing. Did you forget to push a second commit ?

@alpipego
Copy link
Contributor Author

alpipego commented Feb 9, 2022

@alpipego Looks like the example file mentioned in #659 (comment) is missing. Did you forget to push a second commit ?

Sorry, I amended my commit but didn't add the file. It's there now.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alpipego for getting this set up!

Generally looks good to me.

I do think the example file should be renamed though as this is not about the "normal" autoloading, but about preloading the Requests 1.x class names.

examples/autoload.php Outdated Show resolved Hide resolved
examples/autoload.php Outdated Show resolved Hide resolved
src/Autoload.php Outdated Show resolved Hide resolved
@alpipego
Copy link
Contributor Author

Thank you for your notes, @jrfnl.
I have amended my previous commit, I hope this is inline with your expectation. If you'd like me to add micro commits for changes within a PR, I'll gladly switch over to that.

@alpipego alpipego requested a review from jrfnl February 12, 2022 03:53
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alpipego Thanks for the update! And yes, in this case, amending the commits is perfectly fine and makes for a clean commit history, so all good 👍🏻

Aside from the inline remark, there is only one other thing:
What about adding a very simple test for the method. I understand if it feels redundant as there is no logic in the Autoload::get_deprecated_classes() method, but then again, there are also no other safeguards like a type declaration or code within the library calling the method.
So what about a simple test just calling the method and verifying that the returned value is an array and has # number of elements ?

examples/preload-aliases.php Outdated Show resolved Hide resolved
This method will return the array for the class mapping between the
legacy classes and their namespaced 2.x counterparts. The need for this
change came up while using the 2.x version of the library together
with a framework or CMS, that includes a 1.x version of Requests.

In order to prevent the framework from autoloading the 1.x classes,
they must be aliased before accessed. Through exposing the mapping
array, the classes can be aliased before being their implementations
get loaded.

See WordPress#659
@alpipego
Copy link
Contributor Author

I have amended my commit:

  • I have added a test. Since \PHPUnit\Framework\Assert::assertCount checks for Countable/is_iterable, I have omitted testing for the arary type specifically. Please let me know if I should add it.
  • I have reworded the commit message and example to be more framework-agnostic.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alpipego Thanks for the update. Looks good to go to me! I'll give @schlessera a chance to look it over too, but as far as I'm concerned: let's merge this!

examples/preload-aliases.php Show resolved Hide resolved
examples/preload-aliases.php Outdated Show resolved Hide resolved
src/Autoload.php Show resolved Hide resolved
Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>
@jrfnl jrfnl merged commit 97ce85d into WordPress:develop Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants