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

Check which classes can be changed to final #514

Closed
3 tasks done
jrfnl opened this issue Aug 20, 2021 · 1 comment · Fixed by #534
Closed
3 tasks done

Check which classes can be changed to final #514

jrfnl opened this issue Aug 20, 2021 · 1 comment · Fixed by #534
Assignees
Milestone

Comments

@jrfnl
Copy link
Member

jrfnl commented Aug 20, 2021

Tasks:

  • Check via GH search if any of the classes are being extended
  • For all classes where it could reasonably make sense for them to be extended, leave them be, but make all other classes final.
  • Change all protected methods in the final classes to private.
@jrfnl jrfnl added this to the 2.0.0 milestone Aug 20, 2021
@jrfnl jrfnl self-assigned this Aug 20, 2021
@jrfnl
Copy link
Member Author

jrfnl commented Aug 30, 2021

I've done the GH searches for this. Please find my findings below. The finding table includes a proposal for select classes to become final. Where the "Make final ?" column is left empty, I'm not sure whether we should or shouldn't make the class final as I want to prevent Requests from becoming too restrictive.

Opinions welcome.

Type Request 1.x name Can be final Make final ? Search result
class Requests no ... ?
class Requests_Cookie no Search results (0)
class Requests_Exception X no Search results {93,972)
class Requests_Hooks X no Search results (43,085)
class Requests_IDNAEncoder yes no Search results (0)
class Requests_IPv6 yes Search results (0)
class Requests_IRI yes no Search results (0)
class Requests_Response X no Search results (1)
class Requests_Session X no Search results (1)
class Requests_SSL yes Search results (0)
class Requests_Auth_Basic X no Search results (2)
class Requests_Cookie_Jar no Search results (0)
class Requests_Proxy_HTTP yes Search results (0)
class Requests_Response_Headers no Search results (0)
class Requests_Transport_cURL yes Search results (0)
class Requests_Transport_fsockopen yes Search results (0)
class Requests_Utility_CaseInsensitiveDictionary X no Search results (47,367)
class Requests_Utility_FilteredIterator yes Search results (0)
class Requests_Exception_HTTP X no Search results (1,561,406)
class Requests_Exception_Transport X no Search results (46,565)
class Requests_Exception_Transport_cURL yes Search results (0)
class Requests_Exception_HTTP_### yes Search results (0)
class Requests_Exception_HTTP_Unknown yes Search results (0)

New:

Type New Requests 2.x class Make final ?
class Requests\Autoload yes
class Requests\Port (upcoming) yes
class Requests\Exception\InvalidArgument yes

Other

All (non-abstract) test classes will also be made final.

jrfnl added a commit that referenced this issue Nov 5, 2021
In PR #534 (to address #514), a number of classes were made `final`. This effectively broke the BC-layer for the Composer autoloader as that BC-layer relied on the "old" class names extending the new ones.

Fixed now by:
* Using the new custom autoloader - which uses class aliasing, which is not affected by the change to `final` - for the BC-layer for Composer.
* Automatically loading the file via the composer `autoload` directive.

While using the `files` directive in Composer `autoload` is generally a last resort, as this file only registers an autoloader and Composer autoload is used for just that purpose, this should not be a problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant