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

Make StandaloneExtensionManagers configurable #55

Closed
wants to merge 22 commits into from
Closed

Make StandaloneExtensionManagers configurable #55

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
316e9b7
Merge branch 'hotfix/43'
weierophinney Nov 1, 2017
5387abb
Make $extensions property protected
Synchro Nov 10, 2017
b9f3004
Switch to making StandaloneExtensionManager configurable
Synchro Nov 14, 2017
1351b73
Not final
Synchro Nov 14, 2017
e09a172
Add tests for Writer StandaloneExtensionMananger
Synchro Nov 14, 2017
7155b5a
No return value from remove, simplify
Synchro Nov 14, 2017
c431c49
Implement same changes for Writer
Synchro Nov 14, 2017
7a91f4f
Use class pseudoconstants rather than strings for class names
Synchro Nov 14, 2017
849b55d
Fix namespaced names
Synchro Nov 14, 2017
8c3860f
Fix data provider
Synchro Nov 14, 2017
223e867
Fix test provider namespaces
Synchro Nov 14, 2017
4e68bee
Writer, not Reader
Synchro Nov 14, 2017
f807526
Fix data provider
Synchro Nov 14, 2017
f71a00c
Improve docs a little
Synchro Nov 14, 2017
d4a8efe
Add checks when adding extensions
Synchro Nov 14, 2017
a9a5af4
Use mock extension
Synchro Nov 14, 2017
ce6fd8b
Code style
Synchro Nov 14, 2017
3e379f8
Fix wrong exceptions
Synchro Nov 14, 2017
789b4b2
Fix wrong exception
Synchro Nov 14, 2017
4096b6e
More tests
Synchro Nov 14, 2017
1c80524
More tests
Synchro Nov 14, 2017
32982ff
Break out tests
Synchro Nov 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 34 additions & 13 deletions src/Reader/StandaloneExtensionManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,19 @@
class StandaloneExtensionManager implements ExtensionManagerInterface
{
private $extensions = [
'Atom\Entry' => 'Zend\Feed\Reader\Extension\Atom\Entry',
'Atom\Feed' => 'Zend\Feed\Reader\Extension\Atom\Feed',
'Content\Entry' => 'Zend\Feed\Reader\Extension\Content\Entry',
'CreativeCommons\Entry' => 'Zend\Feed\Reader\Extension\CreativeCommons\Entry',
'CreativeCommons\Feed' => 'Zend\Feed\Reader\Extension\CreativeCommons\Feed',
'DublinCore\Entry' => 'Zend\Feed\Reader\Extension\DublinCore\Entry',
'DublinCore\Feed' => 'Zend\Feed\Reader\Extension\DublinCore\Feed',
'Podcast\Entry' => 'Zend\Feed\Reader\Extension\Podcast\Entry',
'Podcast\Feed' => 'Zend\Feed\Reader\Extension\Podcast\Feed',
'Slash\Entry' => 'Zend\Feed\Reader\Extension\Slash\Entry',
'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' => Extension\Atom\Entry::class,
'Atom\Feed' => Extension\Atom\Feed::class,
'Content\Entry' => Extension\Content\Entry::class,
'CreativeCommons\Entry' => Extension\CreativeCommons\Entry::class,
'CreativeCommons\Feed' => Extension\CreativeCommons\Feed::class,
'DublinCore\Entry' => Extension\DublinCore\Entry::class,
'DublinCore\Feed' => Extension\DublinCore\Feed::class,
'Podcast\Entry' => Extension\Podcast\Entry::class,
'Podcast\Feed' => Extension\Podcast\Feed::class,
'Slash\Entry' => Extension\Slash\Entry::class,
'Syndication\Feed' => Extension\Syndication\Feed::class,
'Thread\Entry' => Extension\Thread\Entry::class,
'WellFormedWeb\Entry' => Extension\WellFormedWeb\Entry::class,
];

/**
Expand All @@ -49,4 +49,25 @@ public function get($extension)
$class = $this->extensions[$extension];
return new $class();
}

/**
* Add an extension.
*
* @param string $name
* @param string $class
*/
public function add($name, $class)
{
$this->extensions[$name] = $class;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No validation what is added to the manager?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Xerkus Xerkus Nov 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
* Remove an extension.
*
* @param string $name
*/
public function remove($name)
{
unset($this->extensions[$name]);
}
}
21 changes: 21 additions & 0 deletions src/Writer/StandaloneExtensionManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,25 @@ public function get($extension)
$class = $this->extensions[$extension];
return new $class();
}

/**
* Add an extension.
*
* @param string $name
* @param string $class
*/
public function add($name, $class)
{
$this->extensions[$name] = $class;
}

/**
* Remove an extension.
*
* @param string $name
*/
public function remove($name)
{
unset($this->extensions[$name]);
}
}
13 changes: 13 additions & 0 deletions test/Reader/StandaloneExtensionManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@

class StandaloneExtensionManagerTest extends TestCase
{
/**
* @var StandaloneExtensionManager
*/
private $extensions;

public function setUp()
{
$this->extensions = new StandaloneExtensionManager();
Expand Down Expand Up @@ -80,4 +85,12 @@ public function testEachPluginRetrievalReturnsNewInstance($pluginName, $pluginCl
$this->assertInstanceOf($pluginClass, $test);
$this->assertNotSame($extension, $test);
}

public function testPluginAddRemove()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
$this->extensions->add('Test/Test', 'mytestextension');
$this->assertTrue($this->extensions->has('Test/Test'));
$this->extensions->remove('Test/Test');
$this->assertFalse($this->extensions->has('Test/Test'));
}
}
92 changes: 92 additions & 0 deletions test/Writer/StandaloneExtensionManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
<?php
/**
* Zend Framework (http://framework.zend.com/)
*
* @link http://github.com/zendframework/zf2 for the canonical source repository
* @copyright Copyright (c) 2005-2015 Zend Technologies USA Inc. (http://www.zend.com)
* @license http://framework.zend.com/license/new-bsd New BSD License
*/
namespace ZendTest\Feed\Writer;

use PHPUnit\Framework\TestCase;
use Zend\Feed\Reader\StandaloneExtensionManager;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is supposed to be Writer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

use Zend\Feed\Reader\Extension\WellFormedWeb\Entry;
use Zend\Feed\Reader\Extension\Syndication\Feed;
use Zend\Feed\Reader\ExtensionManagerInterface;

class StandaloneExtensionManagerTest extends TestCase
{
/**
* @var StandaloneExtensionManager
*/
private $extensions;

public function setUp()
{
$this->extensions = new StandaloneExtensionManager();
}

public function testIsAnExtensionManagerImplementation()
{
$this->assertInstanceOf(ExtensionManagerInterface::class, $this->extensions);
}

public function defaultPlugins()
{
return [
'Atom\Renderer\Feed' => ['Atom\Renderer\Feed' => Extension\Atom\Renderer\Feed::class],
'Content\Renderer\Entry' => [
'Content\Renderer\Entry' => Extension\Content\Renderer\Entry::class
],
'DublinCore\Renderer\Entry' => ['DublinCore\Renderer\Entry' => Extension\DublinCore\Renderer\Entry::class],
'DublinCore\Renderer\Feed' => ['DublinCore\Renderer\Feed' => Extension\DublinCore\Renderer\Feed::class],
'ITunes\Entry' => ['ITunes\Entry' => Extension\ITunes\Entry::class],
'ITunes\Feed' => ['ITunes\Feed' => Extension\ITunes\Feed::class],
'ITunes\Renderer\Entry' => ['ITunes\Renderer\Entry' => Extension\ITunes\Renderer\Entry::class],
'ITunes\Renderer\Feed' => ['ITunes\Renderer\Feed' => Extension\ITunes\Renderer\Feed::class],
'Slash\Renderer\Entry' => ['Slash\Renderer\Entry' => Extension\Slash\Renderer\Entry::class],
'Threading\Renderer\Entry' => ['Threading\Renderer\Entry' => Extension\Threading\Renderer\Entry::class],
'WellFormedWeb\Renderer\Entry' => [
'WellFormedWeb\Renderer\Entry' => Extension\WellFormedWeb\Renderer\Entry::class
],
];
}

/**
* @dataProvider defaultPlugins
*/
public function testHasAllDefaultPlugins($pluginName, $pluginClass)
{
$this->assertTrue($this->extensions->has($pluginName));
}

/**
* @dataProvider defaultPlugins
*/
public function testCanRetrieveDefaultPluginInstances($pluginName, $pluginClass)
{
$extension = $this->extensions->get($pluginName);
$this->assertInstanceOf($pluginClass, $extension);
}

/**
* @dataProvider defaultPlugins
*/
public function testEachPluginRetrievalReturnsNewInstance($pluginName, $pluginClass)
{
$extension = $this->extensions->get($pluginName);
$this->assertInstanceOf($pluginClass, $extension);

$test = $this->extensions->get($pluginName);
$this->assertInstanceOf($pluginClass, $test);
$this->assertNotSame($extension, $test);
}

public function testPluginAddRemove()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
$this->extensions->add('Test/Test', 'mytestextension');
$this->assertTrue($this->extensions->has('Test/Test'));
$this->extensions->remove('Test/Test');
$this->assertFalse($this->extensions->has('Test/Test'));
}
}