Make StandaloneExtensionManagers configurable

[Synchro]

This is a simple PR that fixes my request in #54. If you're happy with this, I will add changes to the docs as well, also fixing #44.

[Xerkus]

:-1: from me. Standalone extension manager should not be extended and docs should be corrected not to suggest that.

[Synchro]

Rationale?

[weierophinney]

@Xerkus I disagree. In this case, extension will allow users to not need a container implementation in order to provide more extensions; they can instead extend the standalone implementation to add theirs in.

@Synchro — I'm re-opening and will review.

[Xerkus]

I still oppose extending the class. It have numerous downsides.

• extended class will not be updated along with base StandaloneExtensionManager if it copied the property.
• extended class that is changing behavior of StandaloneExtensionManager should be separate implementation anyway as it will be a coupling of unrelated responsibilities.
• extending limits our ability to change implementation details at will, which is an additional maintenance burden. Not as much of a problem in this simple isolated case but it is there.

I see only two valid options here and none of them is to extend:

• introduce methods to configure extensions property to the StandaloneExtensionManager and forbid extension

• extend functionality via decorator implementing the interface, as I was saying in the related Issue:

  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);
  }
  }

And of course there is always configurable container implementation for more complex needs.

[Synchro]

All this seems to be massive overkill - something that calls itself a plugin manager singularly fails to do anything matching that description - needing to write a convoluted decorator at all smells like a workaround for an inflexible base class. If a provided extension manager doesn't manage extensions, what is it for? A simpler alternative to extend/decorate would be to add an "addExtension" method, so I could say:

$m = new StandalonePluginManager;
$m->addExtension('Media/Entry', 'MyApp\RSS\Media\Entry');

I'm using a framework because it helps me do things with less effort; I want to add one extension, but I have zero interest in writing an extension manager - that's a role I would expect the framework to fulfil.

[Xerkus]

Extension manager is a contract established by the interface. Responsibility of the contract is to manage extensions for zend-feed, ie create, configure and provide. StandaloneExtensionManager is a concrete implementation that is in no way obligated to provide anything beside said contract. Its responsibility is to make zend-feed work standalone without external dependency and it does exactly that. NoopExtensionManager, for example, would do nothing, provide no extensions at all while still being a valid concrete implementation.

If you need flexible configurable extension manager, that role is filled by ExtensionPluginManager. It is already written and supported.

As I said in previous comment, I will support expanding standalone responsibilities by making it configurable. Feel free to open PR for that.

[Xerkus]

Can you also provide same change for Writer?

[Synchro]

OK. I've removed final. Are you happy with the general approach?

[Synchro]

I've noticed that the code style of the extension list is quite different in each. In Reader:

    'Atom\Entry'            => 'Zend\Feed\Reader\Extension\Atom\Entry',

in Writer:

    'Atom\Renderer\Feed'           => Extension\Atom\Renderer\Feed::class,

They are consistent within themselves, but not with each other. It makes writing tests easier using the string style as I don't have to provide a mocked extension class - I know that older versions used the literal style, so I assume there's some history to that.

I noticed that the Writer doesn't have a test file for this, so I'm adding one.

[Xerkus]

::class pseudoconstant is preferred. It is a special case, it does not require class to actually exist. What it does is resolves class using namespace and local imports, returns string. \SomeClass::class and 'SomeClass' are exactly the same, except static analysis will warn of non-existent class in former and won't in latter.

[Synchro]

Hm. Looks like it fails to find classes using pseudoconstants:

Fatal error: Class 'Zend\Feed\Reader\Zend\Feed\Reader\Extension\Podcast\Feed' not found in /home/travis/build/zendframework/zend-feed/src/Reader/StandaloneExtensionManager.php on line 50

I will switch back to strings.

[Synchro]

Uh, I could always just un-break the fully namespaced names...

[Xerkus]

Can you also update docs? For this change only

[Synchro]

Fixing test data provider...

[Xerkus]

This check for Writer extensions makes little sense https://github.com/zendframework/zend-feed/blob/316e9b7db6d2ea6cee15d280e73dd91dfc928b97/src/Writer/ExtensionPluginManager.php#L135-L158

How should it be ported to standalone extension manager?

[Synchro]

All passing now. That OK?

[weierophinney]

Thanks, @Synchro