zendframework / zend-feed

Feed component from Zend Framework
BSD 3-Clause "New" or "Revised" License
166 stars 42 forks source link

Amend StandaloneExtensionManager to allow easier extending #54

Closed Synchro closed 7 years ago

Synchro commented 7 years ago

The docs on extending Zend Feed suggest copy/pasting the whole StandaloneExtensionManager class if you want to change the extensions supported. This seems contrary to good practice, especially given the most likely use case - adding a single extension.

The one obstacle to using it in a better way is that the $extensions property is marked as private, so you can't subclass to amend it, and it also implies you have to copy/paste the standard has/get methods, which seems pointless. Simply changing access to the $extensions property to protected would allow simple addition/change to the list without having to copy/paste other stuff, and will also mean that future changes to the standard extension list (which has happened in the recent past) are supported without code changes. I'd like to be able to do this:

namespace MyApp\RSS;

use Zend\Feed\Reader\StandaloneExtensionManager as Zend_StandaloneExtensionManager;

class StandaloneExtensionManager extends Zend_StandaloneExtensionManager
{
    public function __construct()
    {
        $this->extensions['Media\Entry'] = 'MyApp\RSS\Media\Entry';
    }
}
Xerkus commented 7 years ago

actually, standalone extension manager should be made final but that would be a BC break.

Proper way to "extend" standalone extension manager would be to implement a decorator:

final class MyExtensionManager implements ExtensionManagerInterface
{
    private $decoratedManager;

    private $extensions = [
        'Media\Entry' = MyApp\RSS\Media\Entry::class,
    ];

    public function __construct(ExtensionManagerInterface $em)
    {
        $this->decoratedManager = $em;
    } 

    /**
     * Do we have the extension?
     *
     * @param  string $extension
     * @return bool
     */
    public function has($extension)
    {
        return (array_key_exists($extension, $this->extensions) || $this->decoratedManager->has($extension));
    }

    /**
     * Retrieve the extension
     *
     * @param  string $extension
     * @return mixed
     */
    public function get($extension)
    {
        if (array_key_exists($extension, $this->extensions)) {
            $class = $this->extensions[$extension];
            return new $class();
        }
        return $this->decoratedManager->get($extension);
    }
}

Docs should not recommend extension but composition.

PR to fix docs would be appreciated.

Synchro commented 7 years ago

Fair enough - but it seems crazy that we should need to copy/paste 50 lines of code to add 1 element to an array, in something that calls itself a manager of such things.

froschdesign commented 7 years ago

@Synchro

it seems crazy that we should need to copy/paste 50 lines of code to add 1 element to an array, in something that calls itself a manager of such things.

Yes it's a lot of code, but I think this is only a workaround and no final solution. The example in the documentation is wrong and a simple solution is needed.

Please add your comments here: #44

Synchro commented 7 years ago

I realised I'd written my example derived class wrong, so I corrected it in light of @weierophinney reopening #55.