Skip to content

Registry: Allow using class-strings instead of magic strings#766

Merged
mblaney merged 12 commits intosimplepie:masterfrom
Art4:static-registry
Jan 20, 2023
Merged

Registry: Allow using class-strings instead of magic strings#766
mblaney merged 12 commits intosimplepie:masterfrom
Art4:static-registry

Conversation

@Art4
Copy link
Contributor

@Art4 Art4 commented Nov 17, 2022

As discussed with @jtojnar I finalized #737 for using class-strings instead of strings in Registry class. #737 can be closed.

It also create legacy replacements to provide BC. In future it should also possible to use an interface as $type and provide an implementing class as $class.

jtojnar and others added 8 commits November 3, 2022 13:40
This will make it much simpler to validate typos using static analysis.
Converted using the following command:

    sed -i -E "s/(registry->(register|get_class|create|call)\\()'([^']+)'/\\1\\3::class/g;s/Content_Type_Sniffer::class/Content\\\\Type\\\\Sniffer::class/g;s/Parse_Date::class/Parse\\\\Date::class/g;s/XML_Declaration_Parser::class/XML\\\\Declaration\\\\Parser::class/g" $(find src -name '*.php')
@Art4 Art4 mentioned this pull request Nov 17, 2022
48 tasks
*/
protected $default = [];
protected $default = [
Cache::class => Cache::class,
Copy link
Member

Choose a reason for hiding this comment

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

Why not just use plain list of the class names and then use in_array? Or if this is an optimization just use e.g. true as the values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will allow us to have entries with interface => classes in the future. I have this use case while working on PSR-18 support.

src/Registry.php Outdated
}
if (!empty($this->default[$type])) {
return $this->default[$type];
if (!empty($this->legacy[$type])) {
Copy link
Member

Choose a reason for hiding this comment

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

This will no longer return properly registered class:

class CustomSniffer extends SimplePie\Content\Type\Sniffer {}

$registry->register(SimplePie\Content\Type\Sniffer::class, CustomSniffer::class);
var_dump($registry->get_class(`Content_Type_Sniffer`) instanceof CustomSniffer); // returns false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$this->legacy was only used for registering classes in the deprecated SimplePie::set_*_class() methods if using $registry->register() with $legacy=true is set as 3rd argument. But it was only used for the Cache class.

Your example is indeed working. I've add tests for this cases, see 3f82098#diff-985935790cceb01487a2754f890e4cfbfb4b797b6942c018d9f2374fd9466c6fR121-R138

@Art4 Art4 mentioned this pull request Dec 30, 2022
4 tasks
@mblaney mblaney merged commit 5430a1e into simplepie:master Jan 20, 2023
Art4 added a commit to Art4/simplepie that referenced this pull request Jan 20, 2023
@Art4 Art4 deleted the static-registry branch January 20, 2023 08:29
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.

3 participants