Make StandaloneExtensionManagers configurable #55

Synchro · 2017-11-10T08:06:10Z

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.

Close zendframework#43

Xerkus · 2017-11-10T08:22:43Z

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

Synchro · 2017-11-10T08:29:53Z

Rationale?

weierophinney · 2017-11-13T15:38:31Z

@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 · 2017-11-13T23:04:33Z

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 · 2017-11-14T00:38:31Z

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 · 2017-11-14T01:44:57Z

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 · 2017-11-14T07:41:39Z

src/Reader/StandaloneExtensionManager.php

@@ -9,9 +9,9 @@

 namespace Zend\Feed\Reader;

-class StandaloneExtensionManager implements ExtensionManagerInterface
+final class StandaloneExtensionManager implements ExtensionManagerInterface


We can't make it final, it will require a major version bump as someone is quite likely already extending it.

Xerkus · 2017-11-14T07:43:16Z

Can you also provide same change for Writer?

Synchro · 2017-11-14T07:44:02Z

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

Xerkus · 2017-11-14T07:47:35Z

src/Reader/StandaloneExtensionManager.php

+     * Remove an extension.
+     *
+     * @param string $name
+     * @return bool


It does not need to report the result. It is idempotent operation. void would be fine

Synchro · 2017-11-14T07:53:01Z

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 · 2017-11-14T07:55:15Z

::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 · 2017-11-14T08:09:16Z

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 · 2017-11-14T08:10:12Z

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

Xerkus · 2017-11-14T08:12:16Z

src/Reader/StandaloneExtensionManager.php

-        'Syndication\Feed'      => 'Zend\Feed\Reader\Extension\Syndication\Feed',
-        'Thread\Entry'          => 'Zend\Feed\Reader\Extension\Thread\Entry',
-        'WellFormedWeb\Entry'   => 'Zend\Feed\Reader\Extension\WellFormedWeb\Entry',
+        'Atom\Entry'            => Zend\Feed\Reader\Extension\Atom\Entry::class,


It is resolved to FQCN using normal rules.
Extension\Atom\Entry::class will resolve properly to Zend\Feed\Reader\Extension\Atom\Entry

Xerkus · 2017-11-14T08:14:09Z

Can you also update docs? For this change only

Synchro · 2017-11-14T08:16:59Z

Fixing test data provider...

Xerkus · 2017-11-14T08:29:31Z

test/Writer/StandaloneExtensionManagerTest.php

+namespace ZendTest\Feed\Writer;
+
+use PHPUnit\Framework\TestCase;
+use Zend\Feed\Reader\StandaloneExtensionManager;


This is supposed to be Writer

Ah, my IDE had collapsed the use statements so I didn't see them. Fixed.

froschdesign · 2017-11-14T10:17:09Z

src/Reader/StandaloneExtensionManager.php

     */
    public function remove($name)
    {
-        if (array_key_exists($name, $this->extensions)) {


Any reason, why this check is removed?

it is not needed. unset will work the same and there is no distinction between removing instance and noop.

@Xerkus
You are right, it throws no error.

It's because unset is a language construct, not a real function, and it doesn't care whether the thing you're unsetting exists or not. The check was only there before to set the return value.

froschdesign · 2017-11-14T10:20:38Z

src/Reader/StandaloneExtensionManager.php

+     */
+    public function add($name, $class)
+    {
+        $this->extensions[$name] = $class;


No validation what is added to the manager?

good point. $class should be checked to be instance of Zend\Feed\Reader\Extension\AbstractEntry or Zend\Feed\Reader\Extension\AbstractFeed

I wondered about that - $class can be a string (it was in the components until I changed it), but it might be the name of something that doesn't exist yet, so I don't know that it can be validated at this point.

with check it will be autoloaded when extension is registered. Plugin manager validates on creation.

Xerkus · 2017-11-14T10:35:09Z

This check for Writer extensions makes little sense

zend-feed/src/Writer/ExtensionPluginManager.php

Lines 135 to 158 in 316e9b7

    
           public function validate($plugin) 
        
           { 
        
               if ($plugin instanceof Extension\AbstractRenderer) { 
        
                   // we're okay 
        
                   return; 
        
               } 
        
               if ('Feed' == substr(get_class($plugin), -4)) { 
        
                   // we're okay 
        
                   return; 
        
               } 
        
               if ('Entry' == substr(get_class($plugin), -5)) { 
        
                   // we're okay 
        
                   return; 
        
               } 
        
               throw new InvalidServiceException(sprintf( 
        
                   'Plugin of type %s is invalid; must implement %s\Extension\RendererInterface ' 
        
                   . 'or the classname must end in "Feed" or "Entry"', 
        
                   (is_object($plugin) ? get_class($plugin) : gettype($plugin)), 
        
                   __NAMESPACE__ 
        
               )); 
        
           }

How should it be ported to standalone extension manager?

Synchro · 2017-11-14T14:14:22Z

All passing now. That OK?

Xerkus · 2017-11-14T14:25:58Z

src/Writer/StandaloneExtensionManager.php

@@ -9,6 +9,8 @@

 namespace Zend\Feed\Writer;

+use Zend\ServiceManager\Exception\InvalidServiceException;


You cannot use ServiceManager exceptions in here. Use Zend\Feed\Writer\Exception\InvalidArgumentException

Xerkus · 2017-11-14T14:26:59Z

src/Reader/StandaloneExtensionManager.php

@@ -9,22 +9,24 @@

 namespace Zend\Feed\Reader;

+use Zend\ServiceManager\Exception\InvalidServiceException;


You cannot use ServiceManager exceptions in here. Use Zend\Feed\Reader\Exception\InvalidArgumentException

But that's the type that is thrown?

throw that other type

OK, I see why.

Xerkus · 2017-11-14T14:30:05Z

test/Reader/StandaloneExtensionManagerTest.php

@@ -80,4 +85,13 @@ public function testEachPluginRetrievalReturnsNewInstance($pluginName, $pluginCl
        $this->assertInstanceOf($pluginClass, $test);
        $this->assertNotSame($extension, $test);
    }
+
+    public function testPluginAddRemove()


You need additional tests for expected exception in case of invalid extension class or object of valid extension.

You need additional tests to ensure extensions derived from both allowed classes are accepted

I've added tests for the exceptions, but the check in add is already checking for derived classes, because the class it checks for is abstract, so anything that is_a Extension\AbstractEntry is by definition derived from it, so passing a further derivation wouldn't be testing anything further.

Those should be separate tests with descriptive names for what you are testing for
testAddAcceptsValidExtensionClasses()
testAddRejectsInvalidExtensions()
testAddRejectsInstanceOfValidExtension()

That way we have intent of the test and its implementation details rather than just asserting that current code runs without errors. It makes sure that if code changes at a later stage no intent will be lost.

Xerkus · 2017-11-14T14:33:21Z

test/Writer/StandaloneExtensionManagerTest.php

+        $this->assertNotSame($extension, $test);
+    }
+
+    public function testPluginAddRemove()


Same here, additional tests for expected exception in case of invalid extension class or object of valid extension.

additional tests to ensure extensions derived from allowed class or with classnames ending with 'Feed' or 'Entry' are accepted

Add exceptions to tests

Make StandaloneExtensionManagers configurable

Forward port #55

weierophinney · 2017-12-04T17:58:19Z

Thanks, @Synchro

weierophinney and others added 2 commits November 1, 2017 14:21

Merge branch 'hotfix/43'

316e9b7

Close zendframework#43

Make $extensions property protected

5387abb

Synchro closed this Nov 10, 2017

weierophinney reopened this Nov 13, 2017

Synchro mentioned this pull request Nov 13, 2017

Amend StandaloneExtensionManager to allow easier extending #54

Closed

Switch to making StandaloneExtensionManager configurable

b9f3004

Xerkus reviewed Nov 14, 2017

View reviewed changes

Xerkus changed the base branch from master to develop November 14, 2017 07:42

Not final

1351b73

Xerkus reviewed Nov 14, 2017

View reviewed changes

Synchro added 4 commits November 14, 2017 09:01

Add tests for Writer StandaloneExtensionMananger

e09a172

No return value from remove, simplify

7155b5a

Implement same changes for Writer

c431c49

Use class pseudoconstants rather than strings for class names

7a91f4f

Fix namespaced names

849b55d

Xerkus reviewed Nov 14, 2017

View reviewed changes

Xerkus changed the title ~~Make $extensions property protected~~ Make StandaloneExtensionManagers configurable Nov 14, 2017

Xerkus added this to the 2.9.0 milestone Nov 14, 2017

Xerkus added the enhancement label Nov 14, 2017

Synchro added 2 commits November 14, 2017 09:22

Fix data provider

8c3860f

Fix test provider namespaces

223e867

Xerkus reviewed Nov 14, 2017

View reviewed changes

Synchro added 2 commits November 14, 2017 09:30

Writer, not Reader

4e68bee

Fix data provider

f807526

froschdesign reviewed Nov 14, 2017

View reviewed changes

Synchro added 4 commits November 14, 2017 12:59

Improve docs a little

f71a00c

Add checks when adding extensions

d4a8efe

Use mock extension

a9a5af4

Code style

ce6fd8b

Xerkus suggested changes Nov 14, 2017

View reviewed changes

Synchro added 5 commits November 14, 2017 15:48

Fix wrong exceptions

3e379f8

Add exceptions to tests

Fix wrong exception

789b4b2

More tests

4096b6e

More tests

1c80524

Break out tests

32982ff

Xerkus approved these changes Nov 16, 2017

View reviewed changes

weierophinney added a commit that referenced this pull request Dec 4, 2017

Merge pull request #55 from Synchro/master

ab4e22e

Make StandaloneExtensionManagers configurable

weierophinney added a commit that referenced this pull request Dec 4, 2017

Added CHANGELOG entry for #55

cb984b1

weierophinney closed this in 0c2886c Dec 4, 2017

weierophinney added a commit that referenced this pull request Dec 4, 2017

Merge branch 'hotfix/55' into develop

518afa5

Forward port #55

		@@ -9,6 +9,8 @@

		namespace Zend\Feed\Writer;

		use Zend\ServiceManager\Exception\InvalidServiceException;

		@@ -9,22 +9,24 @@

		namespace Zend\Feed\Reader;

		use Zend\ServiceManager\Exception\InvalidServiceException;

Make StandaloneExtensionManagers configurable #55

Make StandaloneExtensionManagers configurable #55

Conversation

Synchro commented Nov 10, 2017

Xerkus commented Nov 10, 2017

Synchro commented Nov 10, 2017

weierophinney commented Nov 13, 2017

Xerkus commented Nov 13, 2017

Synchro commented Nov 14, 2017

Xerkus commented Nov 14, 2017

Choose a reason for hiding this comment

Xerkus commented Nov 14, 2017

Synchro commented Nov 14, 2017

Choose a reason for hiding this comment

Synchro commented Nov 14, 2017

Xerkus commented Nov 14, 2017 • edited Loading

Synchro commented Nov 14, 2017

Synchro commented Nov 14, 2017

Choose a reason for hiding this comment

Xerkus commented Nov 14, 2017 • edited Loading

Synchro commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xerkus Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Xerkus commented Nov 14, 2017

Synchro commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Dec 4, 2017

Xerkus commented Nov 14, 2017 •

edited

Loading

Xerkus commented Nov 14, 2017 •

edited

Loading

Xerkus Nov 14, 2017 •

edited

Loading