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

Form abstract factory #4358

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
21 changes: 18 additions & 3 deletions library/Zend/Form/Factory.php
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,11 @@ protected function prepareAndInjectHydrator($hydratorOrName, FieldsetInterface $
*/
protected function prepareAndInjectInputFilter($spec, FormInterface $form, $method)
{
if ($spec instanceof InputFilterInterface) {
$form->setInputFilter($spec);
return;
}

if (is_string($spec)) {
if (!class_exists($spec)) {
throw new Exception\DomainException(sprintf(
Expand All @@ -490,6 +495,9 @@ protected function prepareAndInjectInputFilter($spec, FormInterface $form, $meth

$factory = $this->getInputFilterFactory();
$filter = $factory->createInputFilter($spec);
if (method_exists($filter, 'setFactory')) {
$filter->setFactory($factory);
}
$form->setInputFilter($filter);
}

Expand Down Expand Up @@ -528,10 +536,17 @@ protected function prepareAndInjectValidationGroup($spec, FormInterface $form, $
*/
protected function getHydratorFromName($hydratorName)
{
$serviceLocator = $this->getFormElementManager()->getServiceLocator();
$services = $this->getFormElementManager()->getServiceLocator();

if ($services && $services->has('HydratorManager')) {
$hydrators = $services->get('HydratorManager');
if ($hydrators->has($hydratorName)) {
return $hydrators->get($hydratorName);
}
}

if ($serviceLocator && $serviceLocator->has($hydratorName)) {
return $serviceLocator->get($hydratorName);
if ($services && $services->has($hydratorName)) {
return $services->get($hydratorName);
}

if (!class_exists($hydratorName)) {
Expand Down
154 changes: 154 additions & 0 deletions library/Zend/Form/FormAbstractFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
<?php
/**
* Zend Framework (http://framework.zend.com/)
*
* @link http://github.com/zendframework/zf2 for the canonical source repository
* @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
* @license http://framework.zend.com/license/new-bsd New BSD License
*/

namespace Zend\Form;

use Zend\InputFilter\InputFilterInterface;
use Zend\ServiceManager\AbstractFactoryInterface;
use Zend\ServiceManager\ServiceLocatorInterface;

class FormAbstractFactory implements AbstractFactoryInterface
{
/**
* @var string Top-level configuration key indicating forms configuration
*/
protected $configKey = 'form_manager';
Copy link
Contributor

Choose a reason for hiding this comment

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

forms wouldn't make more sense here? Since everything that will go under this key are just forms specifications!?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm ambivalent. I went with something that mirrored the idea of
service_manager.

On Tuesday, April 30, 2013, Josias Duarte Busiquia wrote:

In library/Zend/Form/FormAbstractFactory.php:

  • * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
  • * @license http://framework.zend.com/license/new-bsd New BSD License
  • */
    +
    +namespace Zend\Form;
    +
    +use Zend\InputFilter\InputFilterInterface;
    +use Zend\ServiceManager\AbstractFactoryInterface;
    +use Zend\ServiceManager\ServiceLocatorInterface;
    +
    +class FormAbstractFactory implements AbstractFactoryInterface
    +{
  • /**
  • \* @var string Top-level configuration key indicating forms configuration
    
  • */
    
  • protected $configKey = 'form_manager';

forms wouldn't make more sense here? Since everything that will go under
this key are just forms specifications!?


Reply to this email directly or view it on GitHub/~https://github.com//pull/4358/files#r4024798
.

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/


/**
* @var Factory Form factory used to create forms
*/
protected $factory;

/**
* @var string Service prefix necessary for abstract factory to trigger
*/
protected $servicePrefix = 'Form\\';
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason to force the prefix? IMHO this limit us, e. g. if I want to prefix my forms with the module name.

Copy link
Member Author

Choose a reason for hiding this comment

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

A prefix helps prevent false positive lookups, and naming collisions. We're
using this pattern elsewhere, too. We could potentially change this before
the rc.

On Tuesday, April 30, 2013, Josias Duarte Busiquia wrote:

In library/Zend/Form/FormAbstractFactory.php:

+class FormAbstractFactory implements AbstractFactoryInterface
+{

  • /**
  • \* @var string Top-level configuration key indicating forms configuration
    
  • */
    
  • protected $configKey = 'form_manager';
  • /**
  • \* @var Factory Form factory used to create forms
    
  • */
    
  • protected $factory;
  • /**
  • \* @var string Service prefix necessary for abstract factory to trigger
    
  • */
    
  • protected $servicePrefix = 'Form';

What's the reason to force the prefix? IMHO this limit us, e. g. if I want
to prefix my forms with the module name.


Reply to this email directly or view it on GitHub/~https://github.com//pull/4358/files#r4024876
.

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/

Copy link
Contributor

Choose a reason for hiding this comment

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

I vote for removing it, and leave the job for developers, good naming conventions can take of that easily. I say that because at first glance I thought about naming my forms like MyModule\Form\ContactForm, and with the prefix I would be restricted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not true. You'd still be able to name them that; you'd simply alias them:

return array(
    'form_manager' => array(
        'Foo' => array( /* ... */),
    ),
    'service_manager' => array(
        'Form\Foo' => 'MyModule\Form\ContactForm',
    ),
);

Note: completely untested. However, I think that Form\Foo becomes the "requested name", which is what we check against, and MyModule\Form\ContactForm would be the canonicalized name.

That said, if you're using this functionality, you're creating what is at heart a configuration-driven form. It will either be of the generic type Zend\Form\Form, or of a descendent type based on the "type" you provide in configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

I came across this issue because my code breaks when using full names like @wryck7 uses MyModule\Form\ContactForm. I also use this way of defining and found out I am forced to use aliases now to make my code working again in ZF 2.2.0.

@weierophinney I don't see how aliasing is a simple way to do this. I use full names which very clearly show which class I use in the call, furthermore full names prevent accidental usage of duplicate aliases. Now I am forced to use aliases and make up new names that won't collide with other aliases. In my opinion it's overkill, because my naming was already guaranteed unique because of the full name: Module\Form\FormName.

I don't know if this code is still used, but my code breaks with full names and only aliasing them fixes this...

Copy link
Contributor

Choose a reason for hiding this comment

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

@Martin-P The prefix was removed before the release so the full name should be working. Open an issue and detail what you're doing and how it is breaking.

Copy link
Contributor

Choose a reason for hiding this comment

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

I finally figured out what caused the problem. It was a config file config/autoload/override.local.php with some test config in it. Only this array in it is enough to make the form_elements config key useless:

return array(
    'di' => array(
    ),
);

It took me many hours to find out what huge problems this small file caused. Is this intended behaviour? If not, I will open an issue for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you provide a form configuration that demonstrates this? We did a
number of changes prior to 2.2 that should have alleviated interop issues
with DI, and I'm curious if we missed an edge case.

On Saturday, June 1, 2013, Martin-P wrote:

In library/Zend/Form/FormAbstractFactory.php:

+class FormAbstractFactory implements AbstractFactoryInterface
+{

  • /**
  • \* @var string Top-level configuration key indicating forms configuration
    
  • */
    
  • protected $configKey = 'form_manager';
  • /**
  • \* @var Factory Form factory used to create forms
    
  • */
    
  • protected $factory;
  • /**
  • \* @var string Service prefix necessary for abstract factory to trigger
    
  • */
    
  • protected $servicePrefix = 'Form';

I finally figured out what caused the problem. It was a config file
config/autoload/override.local.php with some test config in it. Only this
array in it is enough to make the form_elements config key useless:

return array(
'di' => array(
),);

It took me many hours to find out what huge problems this small file
caused. Is this intended behaviour? If not, I will open an issue for this.


Reply to this email directly or view it on GitHub/~https://github.com//pull/4358/files#r4490495
.

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/

Copy link
Contributor

Choose a reason for hiding this comment

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

@weierophinney At that moment I thought I was doing something wrong, but issue #6212 (comment) shows the same issue and reminded me of this issue. It does look like a DI issue after all.


/**
* Can we create the requested service?
*
* @param ServiceLocatorInterface $services
* @param string $name Service name (as resolved by ServiceManager)
* @param string $rName Name by which service was requested
* @return bool
*/
public function canCreateServiceWithName(ServiceLocatorInterface $services, $name, $rName)
{
if (!$services->has('Config')) {
return false;
}

$prefixLength = strlen($this->servicePrefix);
if (strlen($rName) < $prefixLength
|| substr($rName, 0, $prefixLength) !== $this->servicePrefix
) {
return false;
}

$config = $services->get('Config');
if (!isset($config[$this->configKey])
|| !is_array($config[$this->configKey])
|| empty($config[$this->configKey])
) {
return false;
}

$config = $config[$this->configKey];

$serviceName = substr($rName, $prefixLength);
if (!isset($config[$serviceName])
|| !is_array($config[$serviceName])
|| empty($config[$serviceName])
) {
return false;
}

return true;
}

/**
* Create a form
*
* @param ServiceLocatorInterface $services
* @param string $name Service name (as resolved by ServiceManager)
* @param string $rName Name by which service was requested
* @return Form
*/
public function createServiceWithName(ServiceLocatorInterface $services, $name, $rName)
{
$serviceName = substr($rName, strlen($this->servicePrefix));
$config = $services->get('Config');

$config = $config[$this->configKey][$serviceName];

$factory = $this->getFormFactory($services);
$this->marshalInputFilter($config, $services, $factory);

return $factory->createForm($config);
}

/**
* Retrieve the form factory, creating it if necessary
*
* @param ServiceLocatorInterface $services
* @return Factory
*/
protected function getFormFactory(ServiceLocatorInterface $services)
{
if ($this->factory instanceof Factory) {
return $this->factory;
}

$elements = null;
if ($services->has('FormElementManager')) {
$elements = $services->get('FormElementManager');
}

$this->factory = new Factory($elements);
return $this->factory;
}

/**
* Marshal the input filter into the configuration
*
* If an input filter is specified:
* - if the InputFilterManager is present, checks if it's there; if so,
* retrieves it and resets the specification to the instance.
* - otherwise, pulls the input filter factory from the form factory, and
* attaches the FilterManager and ValidatorManager to it.
*
* @param array $config
* @param ServiceLocatorInterface $services
* @param Factory $formFactory
*/
protected function marshalInputFilter(array &$config, ServiceLocatorInterface $services, Factory $formFactory)
{
if (!isset($config['input_filter'])) {
return;
}

if ($config['input_filter'] instanceof InputFilterInterface) {
return;
}

if (is_string($config['input_filter'])
&& $services->has('InputFilterManager')
) {
$inputFilters = $services->get('InputFilterManager');
if ($inputFilters->has($config['input_filter'])) {
$config['input_filter'] = $inputFilters->get($config['input_filter']);
return;
}
}

$inputFilterFactory = $formFactory->getInputFilterFactory();
$inputFilterFactory->getDefaultFilterChain()->setPluginManager($services->get('FilterManager'));
$inputFilterFactory->getDefaultValidatorChain()->setPluginManager($services->get('ValidatorManager'));
}
}
85 changes: 0 additions & 85 deletions library/Zend/Form/FormAbstractServiceFactory.php

This file was deleted.

3 changes: 3 additions & 0 deletions library/Zend/Mvc/Service/ServiceListenerFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ class ServiceListenerFactory implements FactoryInterface
'Zend\View\Resolver\AggregateResolver' => 'ViewResolver',
'Zend\View\Resolver\ResolverInterface' => 'ViewResolver',
),
'abstract_factories' => array(
'Zend\Form\FormAbstractFactory',
),
);

/**
Expand Down
29 changes: 29 additions & 0 deletions tests/ZendTest/Form/FactoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Zend\Form\Factory as FormFactory;
use Zend\InputFilter;
use Zend\ServiceManager\ServiceManager;
use Zend\Stdlib\Hydrator\HydratorPluginManager;

/**
* @category Zend
Expand Down Expand Up @@ -314,6 +315,18 @@ public function testCanCreateFormsWithInputFilterSpecifications()
}
}

public function testCanCreateFormsWithInputFilterInstances()
{
$filter = new TestAsset\InputFilter();
$form = $this->factory->createForm(array(
'name' => 'foo',
'input_filter' => $filter,
));
$this->assertInstanceOf('Zend\Form\FormInterface', $form);
$test = $form->getInputFilter();
$this->assertSame($filter, $test);
}

public function testCanCreateFormsAndSpecifyHydrator()
{
$form = $this->factory->createForm(array(
Expand All @@ -325,6 +338,22 @@ public function testCanCreateFormsAndSpecifyHydrator()
$this->assertInstanceOf('Zend\Stdlib\Hydrator\ObjectProperty', $hydrator);
}

public function testCanCreateFormsAndSpecifyHydratorManagedByHydratorManager()
{
$hydrators = new HydratorPluginManager();
$services = $this->factory->getFormElementManager()->getServiceLocator();
$hydrators->setServiceLocator($services);
$services->setService('HydratorManager', new HydratorPluginManager());

$form = $this->factory->createForm(array(
'name' => 'foo',
'hydrator' => 'ObjectProperty',
));
$this->assertInstanceOf('Zend\Form\FormInterface', $form);
$hydrator = $form->getHydrator();
$this->assertInstanceOf('Zend\Stdlib\Hydrator\ObjectProperty', $hydrator);
}

public function testCanCreateHydratorFromArray()
{
$form = $this->factory->createForm(array(
Expand Down
Loading