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

Ensure input_filters config is honored in non-zend-mvc contexts #137

Conversation

weierophinney
Copy link
Member

Per https://discourse.zendframework.com/t/validatormanager-not-calling-custom-validator-factory/109/5?u=matthew the input_filters config key is not honored currently unless the application is within a zend-mvc context. This is due to the fact that Zend\InputFilter\Module wires configuration for the Zend\ModuleManager\Listener\ServiceListener in order to push merged service configuration into the plugin during bootstrap; no similar logic is available when not in a zend-mvc context, however.

This patch fixes that situation by modifying the InputFilterPluginManagerFactory to do the following:

  • If a ServiceListener service exists, it returns the plugin manager immediately (old behavior).
  • Otherwise, it checks for the config service, and, if found, a input_filters key with an array value. When found, it feeds that value to a Zend\ServiceManager\Config instance and uses that to configure the plugin manager before returning it.

Per https://discourse.zendframework.com/t/validatormanager-not-calling-custom-validator-factory/109/5?u=matthew
the `input_filters` config key is not honored currently unless the
application is within a zend-mvc context. This is due to the fact that
`Zend\InputFilter\Module` wires configuration for the
`Zend\ModuleManager\Listener\ServiceListener` in order to push merged
service configuration into the plugin during bootstrap; no similar logic
is available when not in a zend-mvc context, however.

This patch fixes that situation by modifying the
`InputFilterPluginManagerFactory` to do the following:

- If a `ServiceListener` service exists, it returns the plugin manager
  immediately (old behavior).
- Otherwise, it checks for the `config` service, and, if found, a
  `input_filters` key with an array value. When found, it feeds that value
  to a `Zend\ServiceManager\Config` instance and uses that to configure
  the plugin manager before returning it.
@weierophinney weierophinney changed the title Ensure validators config is honored in non-zend-mvc contexts Ensure input_filters config is honored in non-zend-mvc contexts May 16, 2017
@weierophinney
Copy link
Member Author

Failing tests are against latest target, and are due to PHPUnit version mismatch issues; failures only affect InputFilterPluginManagerCompatibilityTest; I've run the new tests manually with latest dependencies, and they pass as expected.

@svycka
Copy link
Contributor

svycka commented May 17, 2017

@weierophinney does this mean that we no longer require to write our own factory for zend-expressive? also do you plan to add this to other plugin managers like validator, filter, view, controllers and others?

@froschdesign
Copy link
Member

@svycka

also do you plan to add this to other plugin managers like validator, filter, view, controllers and others?

Already done:

zendframework/zend-filter/pull/56
zendframework/zend-form#164
zendframework/zend-validator#168
zendframework/zend-log#74
zendframework/zend-i18n#74
zendframework/zend-hydrator#59

@svycka
Copy link
Contributor

svycka commented May 17, 2017

oh nice didn't see this

Copy link
Member

@froschdesign froschdesign left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@froschdesign froschdesign added this to the 2.7.4 milestone May 17, 2017
@mtymek
Copy link

mtymek commented May 17, 2017

I would also add the factory for Zend\InputFilter\Factory that injects filter and validator plugin managers to appropriate chain.

Here's how I implemented it in my library:

use Interop\Container\ContainerInterface;
use Zend\Filter\FilterPluginManager;
use Zend\InputFilter\Factory;
use Zend\InputFilter\InputFilterPluginManager;
use Zend\Validator\ValidatorPluginManager;

class InputFilterFactoryFactory
{
    public function __invoke(ContainerInterface $container)
    {
        $factory = new Factory($container->get(InputFilterPluginManager::class));
        $factory->getDefaultFilterChain()->setPluginManager($container->get(FilterPluginManager::class));
        $factory->getDefaultValidatorChain()->setPluginManager($container->get(ValidatorPluginManager::class));

        return $factory;
    }
}

This is required to configure populate input filter using add method with array definition:

class ContactInputFilter extends InputFilter
{
    public function __construct()
    {
        $this->add(
            [
                'name' => 'first_name',
                'filters' => [
                    ['name' => 'Foo'],
                ],
                'validators' => [
                    ['name' => 'Bar'],
                ],
            ]
        );

        $this->add(
            [
                'name' => 'content',
                'filters' => [
                    ['name' => 'StringTrim'],
                ],
            ]
        );
    }
}

@svycka
Copy link
Contributor

svycka commented May 17, 2017

@mtymek are you sure this is required? I don't remember doing this. Maybe you not added filter and validators config providers to config?

@mtymek
Copy link

mtymek commented May 17, 2017

@froschdesign lines you mentioned will only work under zend-mvc, because FQCN is not used when pulling plugin manager for the container:

$factory->getDefaultFilterChain()->setPluginManager($container->get('FilterManager'));

My code uses FilterPluginManager::class instead of just 'FilterManager'. Do you see the nuance? ;-)

@svycka
Copy link
Contributor

svycka commented May 17, 2017

@mtymek then I think this should be solved here https://github.com/zendframework/zend-filter/blob/master/src/ConfigProvider.php#L33 by adding FilterPluginManager::class and making 'FilterManager' as alias. This would solve this no?

@froschdesign
Copy link
Member

@mtymek

lines you mentioned will only work under zend-mvc

…or you have registered already these managers.

@mtymek
Copy link

mtymek commented May 17, 2017

@svycka indeed, this could work as well. Maybe it would be enough to add the aliases to ConfigProvider class?

@froschdesign I don't understand your remark. Can you elaborate?

@weierophinney
Copy link
Member Author

@mtymek — regarding this:

$factory->getDefaultFilterChain()->setPluginManager($container->get(FilterPluginManager::class));
$factory->getDefaultValidatorChain()->setPluginManager($container->get(ValidatorPluginManager::class));

I'm not sure why you need that at all. zend-filter defines the FilterManager service in its ConfigProvider, and zend-validator defines ValidatorManager in its own. InputFilterPluginManager pulls these two services when populating the Factory instance with the default plugin managers. Your changes above are using service names we do not define in the existing ConfigProvider definitions, which means they shouldn't work at all... unless your module is defining them.

I agree that we should likely add the FQCN as the service name pointing to the factory, and the shorter names as aliases to those, but after the proposed changes in this PR and the new tags I've created on the other components, it should all work seamlessly.

weierophinney added a commit that referenced this pull request May 18, 2017
@weierophinney weierophinney merged commit 5b652bc into zendframework:master May 18, 2017
weierophinney added a commit that referenced this pull request May 18, 2017
weierophinney added a commit that referenced this pull request May 18, 2017
@weierophinney weierophinney deleted the hotfix/input_filters-service-config branch May 18, 2017 13:37
@mtymek
Copy link

mtymek commented May 18, 2017

Right :) The reason you see it is that is that I working on my lib before ConfigProvider was available in zend-inputfilter. Sorry for adding more confusion!

@weierophinney
Copy link
Member Author

@mtymek No worries! Glad it's all sorted!

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

Successfully merging this pull request may close these issues.

4 participants