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

Remove broken BC code in Registry #764

Merged
merged 1 commit into from
Dec 2, 2022

Conversation

Art4
Copy link
Contributor

@Art4 Art4 commented Nov 7, 2022

While working on the Registry class I noted this BC code that actually never worked. It was introduced in f6e63e6 (version 1.3).

The code was intended to fix a BC break in the Locator::__construct() method. But because the switch looks for locator instead of (uppercase) Locator there was no chance to register a BC class that matches this requirement. Please note also the wrong lower case in $this->get_class('file') and $this->get_class('content_type_sniffer') that would always return null instead of a class name. Also I could not find any complaints about a BC break for the changed parameter in the Locator::__construct() method.

Therefor I propose to remove this BC code, that actually never worked.

Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

Looks good – the only way this could work if someone used the Registry newly introduced in f6e63e6 with the legacy class and lowercase name $simplepie->registry->register('locator', $class, true). But as you noticed that is not possible, since the lowercase $type argument would not be a key in the Registry::$defaults property so registration would fail on not finding a parent class to check subclass relationship for.

@jtojnar
Copy link
Contributor

jtojnar commented Nov 13, 2022

Now, if only we could drop the other legacy alias:

5f107f3#diff-c09efaf19f27f39a1f6829e8ded114062e329cb15f8ce1d65f1f0a6c6bb159bd

Unfortunately, that one can be relied on externally so we would need to deprecate it first. While at it, we should probably also deprecate Simplepie::set_*_class() methods, since that is the place that legacy classes are registered.

@Art4
Copy link
Contributor Author

Art4 commented Nov 14, 2022

Now, if only we could drop the other legacy alias:

5f107f3#diff-c09efaf19f27f39a1f6829e8ded114062e329cb15f8ce1d65f1f0a6c6bb159bd

I also thought about removing that, but since the whole Cache class is already deprecated, it won't bother me.

Unfortunately, that one can be relied on externally so we would need to deprecate it first. While at it, we should probably also deprecate Simplepie::set_*_class() methods, since that is the place that legacy classes are registered.

The Simplepie::set_*_class() methods are already deprecated since SimplePie 1.3, see the phpdoc template in https://github.com/simplepie/simplepie/blob/1.3/library/SimplePie.php#L887-L894
I will made that clearer in a future PR.

@mblaney
Copy link
Member

mblaney commented Dec 2, 2022

thanks @Art4 good catch!

@mblaney mblaney merged commit fdf50d6 into simplepie:master Dec 2, 2022
@Art4 Art4 deleted the remove_broken_bc_code_in_registry branch December 2, 2022 08:03
Art4 added a commit to Art4/simplepie that referenced this pull request Dec 2, 2022
mblaney pushed a commit that referenced this pull request Jan 20, 2023
* bump version to 1.8.0

* Update CHANGELOG.md

* Fix version tags in deprecated messages

* fix version in old deprecation messages

* Fix typo

see comment from @jtojnar in #752

* Add comment for DataCache interface

see comment from @jtojnar in #752

* Update CHANGELOG.md for #760, #764 and #765

* Update CHANGELOG.md for #762, #767 and #763

* Update CHANGELOG.md for #768 and #770

* Update release date

* Update CHANGELOG.md for #769 and #771

* Update CHANGELOG.md for #766
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants