Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Amend StandaloneExtensionManager to allow easier extending #54

Closed
Synchro opened this issue Nov 10, 2017 · 4 comments
Closed

Amend StandaloneExtensionManager to allow easier extending #54

Synchro opened this issue Nov 10, 2017 · 4 comments

Comments

@Synchro
Copy link
Contributor

Synchro commented Nov 10, 2017

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
Copy link
Member

Xerkus commented Nov 10, 2017

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 Synchro closed this as completed Nov 10, 2017
@Synchro
Copy link
Contributor Author

Synchro commented Nov 10, 2017

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
Copy link
Member

@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
Copy link
Contributor Author

Synchro commented Nov 13, 2017

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants