Registry: Allow using class-strings instead of magic strings

[jtojnar]

Depends on #736

This will make it much simpler to validate typos using static analysis.

[jtojnar]

Not sure if this is worth pursuing, the registry is pretty ugly design pattern. Ideally we would switch to constructor-based dependency injection. But that would probably be a large BC break.

[Art4]

What do you think about implementing PSR-11 container interface with SimplePie\Registry? Or even create a new Container class that would internally used by SimplePie\Registry? We could create a new method SimplePie\SimplePie::set_container(Psr\Container\ContainerInterface $container) to let the user inject their own container.

Inside SimplePie\Registry we have to deprecate the methods register(), get_class(), create() and call(). And we have to deprecate the static methods in SimplePie\Misc and recreate them as instance methods.

This way we could get rid of the old Registry in v2 without breaking BC.

[jtojnar]

What do you think about implementing PSR-11 container interface with SimplePie\Registry?

That might standardize the interface but I probably would not bother. My actual beef is with the registry itself as it is an instance of the service locator (anti)pattern. PSR-11 itself warns against that.

If SimplePie was part of my project, I would probably remove registry altogether, turn as much classes as possible into plain data objects that could be created directly with new, have classes take dependencies as constructor parameters (or factories for classes like Item), and let the project handle creation of objects through its own dependency injection container. Though the factories would still probably depend on service locator. And I am not sure if this is really how we are supposed to build libraries.

Or even create a new Container class that would internally used by SimplePie\Registry? We could create a new method SimplePie\SimplePie::set_container(Psr\Container\ContainerInterface $container) to let the user inject their own container.

SimplePie does not have set_registry method and creates a Registry instance in the constructor so I would consider it as final (even thought it is not technically marked final and the registry property is not marked access private.) So I do not see a benefit to introducing a separate class from BC point of view. We could just implement ContainerInterface on Registry and switch all classes to use that.

If someone started passing custom ContainerInterface to SimplePie and were also overriding other SimplePie classes, they would need to switch them away from the deprecated Registry methods but I would not consider that a BC break since custom containers would be a new feature – the Registry would keep the deprecated methods.

And we have to deprecate the static methods in SimplePie\Misc and recreate them as instance methods.

That feels a bit icky to me. Those are independent functions that only need to be in a class because PHP does not support function autoloading. I would probably go with adding constructor to Misc and continue calling the methods statically. Or even just use the the functions directly – there should not really be a reason to override functions like Misc::codepoint_to_utf8 so I would not consider tight coupling to be a code smell here.

[Art4]

Yes, you are right.

Maybe we should keep the removal of the Registry class for later. First, we can make the cache implementations and SimplePie\File obsolete through PSR-16 and PSR-18 (see #731). After that, we can better see what functions are actually needed by SimplePie\Registry and then better plan how to replace that with constructor parameters or factories.

there should not really be a reason to override functions like Misc::codepoint_to_utf8 so I would not consider tight coupling to be a code smell here.

Still, there are places like this one right now:

$parsed_feed_url = $this->registry->call('Misc', 'parse_url', array($this->feed_url));

https://github.com/simplepie/simplepie/blob/1.6.0/library/SimplePie.php#L1441

But I agree. We could move the functionality to a new Utils class and mark the calls via Registry as deprecated.

[Art4]

@jtojnar I would like to see this PR merged. Are you interested in finalize it or could I do that?

[jtojnar]

@Art4 I have rebased the patchset but probably will not have time to make it mergeable anytime soon. Feel free to take over.

[Art4]

Thank you very much. 👍

[Art4]

@mblaney This PR can be closed.