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

[BUG] An alias "filerename" was requested but no service could be found ZF2.5.3 #14

Merged
merged 2 commits into from
Feb 16, 2016
Merged

[BUG] An alias "filerename" was requested but no service could be found ZF2.5.3 #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d28ddba
Merge property aliases instead of replacing
Feb 10, 2016
8c7ef68
update teste
Feb 10, 2016
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
4 changes: 3 additions & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@
"zendframework/zend-stdlib": "~2.5"
},
"require-dev": {
"zendframework/zend-filter": "~2.5",
"zendframework/zend-filter": "~2.6",
"zendframework/zend-i18n": "~2.5",
"zendframework/zend-servicemanager": "~2.5",
"zendframework/zend-validator": "~2.5",
"zendframework/zend-progressbar": "~2.5",
Copy link
Member

Choose a reason for hiding this comment

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

Needed? In which case?

Copy link
Member

Choose a reason for hiding this comment

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

Interestingly, in ZendTest\File\Transfer\Adatper\HttpTest. Missing requirement, evidently.

Copy link
Member

Choose a reason for hiding this comment

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

(and that test requires it in order to test Zend\File\Transfer\Adapter\Http)

"zendframework/zend-session": "~2.5",
Copy link
Member

Choose a reason for hiding this comment

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

Needed? In which case?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I cannot find a requirement for this anywhere.

"fabpot/php-cs-fixer": "1.7.*",
"phpunit/PHPUnit": "~4.0"
},
Expand Down
28 changes: 16 additions & 12 deletions src/Transfer/Adapter/FilterPluginManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
namespace Zend\File\Transfer\Adapter;

use Zend\Filter\FilterPluginManager as BaseManager;
use Zend\Filter\File;

/**
* Plugin manager implementation for the filter chain.
Expand All @@ -20,16 +21,19 @@
*/
class FilterPluginManager extends BaseManager
{
/**
* Default set of filters
*
* @var array
*/
protected $aliases = [
'decrypt' => 'filedecrypt',
'encrypt' => 'fileencrypt',
'lowercase' => 'filelowercase',
'rename' => 'filerename',
'uppercase' => 'fileuppercase',
];

public function __construct($configOrContainerInstance = null, array $v3config = [])
Copy link
Member

Choose a reason for hiding this comment

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

Constructor needs documentation docblock

Copy link
Member

Choose a reason for hiding this comment

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

v3config naming is a bit confusing

Copy link
Member

Choose a reason for hiding this comment

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

@Ocramius that's the naming current under zend-servicemanager; in v3, it's a configuration array; v2 doesn't accept a second argument. Hence, $v3config. (Hey, it's not great, but I got a green light on review for that...)

Copy link
Member

Choose a reason for hiding this comment

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

I got a green light on review for that

@weierophinney that's ok, didn't know :-)

{
parent::__construct($configOrContainerInstance, $v3config);

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 a BC break

Copy link
Member

Choose a reason for hiding this comment

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

@Ocramius yes and no.

Prior to zend-filter 2.6.0, Zend\Filter\FilterPluginManager did not define any $aliases, which made adding them to this extension safe. However, part of the 2.6.0 update was separating invokables into aliases + factories. That change has made it so that adding the aliases property in this extension breaks expectations, as other filters expected to be present are missing. The approach being done in this patch is to merge this "default set" with those defined in the parent, via the constructor, which is both backwards and forwards compatible.

Copy link
Member

Choose a reason for hiding this comment

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

Still a BC break from a consumer of this library perspective though :-\

$this->aliases = array_merge(array(
Copy link
Member

Choose a reason for hiding this comment

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

You can use the short array syntax. Remember that multi-line parameters should stay on a line of their own as per PSR-7

Copy link
Member

Choose a reason for hiding this comment

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

@Ocramius wants to say PSR-2 :)

Copy link
Member

Choose a reason for hiding this comment

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

Long night :P

'decrypt' => File\Decrypt::class,
'encrypt' => File\Encrypt::class,
'lowercase' => File\LowerCase::class,
'rename' => File\Rename::class,
'uppercase' => File\UpperCase::class
), $this->aliases);
}

}

Copy link
Member

Choose a reason for hiding this comment

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

revert

2 changes: 1 addition & 1 deletion test/Transfer/Adapter/AbstractTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ public function testGetFilterShouldReturnNullWhenNoMatchingIdentifierExists()

public function testAdapterShouldAllowPullingFiltersByFile()
{
$this->adapter->addFilter('Boolean', 1, 'foo');
$this->adapter->addFilter('Boolean', [1], 'foo');
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a BC break

Copy link
Member

Choose a reason for hiding this comment

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

On investigation, this is due to how "creation options" are handled in v2 with the invokable factory. I think there may be a way to work around it; testing something now.

$filters = $this->adapter->getFilters('foo');
$this->assertEquals(1, count($filters));
$filter = array_shift($filters);
Expand Down
6 changes: 4 additions & 2 deletions test/Transfer/Adapter/HttpTestMockAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
*/
class HttpTestMockAdapter extends Adapter\Http
{
static $aa = true;
Copy link
Member

Choose a reason for hiding this comment

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

Can this be given a better name?


public function __construct()
{
self::$callbackApc = ['ZendTest\File\Transfer\Adapter\HttpTestMockAdapter', 'apcTest'];
Expand All @@ -36,7 +38,7 @@ public function isValidParent($files = null)

public static function isApcAvailable()
{
return true;
return static::$aa;
}

public static function apcTest($id)
Expand All @@ -50,7 +52,7 @@ public static function uPTest($id)
}

public function switchApcToUP()
{
{ static::$aa = false;
Copy link
Member

Choose a reason for hiding this comment

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

Should be on a new line. Also: why is this change required?

Copy link
Member

Choose a reason for hiding this comment

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

I've pulled the changes in locally, and none of the changes to this particular class are necessary; tests pass with and without them.

self::$callbackApc = null;
self::$callbackUploadProgress = ['ZendTest\File\Transfer\Adapter\HttpTestMockAdapter', 'uPTest'];
}
Expand Down