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

set correct Factory when autoAddInvokableClass=true #134

Closed
wants to merge 3 commits into from
Closed

set correct Factory when autoAddInvokableClass=true #134

wants to merge 3 commits into from

Conversation

turrsis
Copy link
Contributor

@turrsis turrsis commented Jan 19, 2017

set correct Zend\Form\ElementFactory (instead of Zend\ServiceManager\Factory\InvokableFactory) when FormElementManager::autoAddInvokableClass = true.
fix for #114

@akrabat
Copy link
Contributor

akrabat commented Feb 23, 2017

Can you explain what problem this fixes?

@turrsis
Copy link
Contributor Author

turrsis commented Feb 24, 2017

When you get element with options :

$element = FormElementManager::get(Element::class, ['key' => 'value'])
$element->getName() === ['key' => 'value']

this is wrong

@@ -0,0 +1,25 @@
<?php
/**
* Zend Framework (http://framework.zend.com/)
Copy link
Member

Choose a reason for hiding this comment

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

Please update the license header and use:

/**
 * @see       https://github.com/zendframework/zend-navigation for the canonical source repository
 * @copyright Copyright (c) 2017 Zend Technologies USA Inc. (http://www.zend.com)
 * @license   https://github.com/zendframework/zend-form/blob/master/LICENSE.md New BSD License
 */


if (! $this->has($name)) {
if (! $this->autoAddInvokableClass || ! class_exists($name)) {
throw new Exception\ServiceNotFoundException(sprintf(
Copy link
Member

Choose a reason for hiding this comment

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

ServiceNotFoundException is not imported.

@@ -222,6 +222,13 @@ public function testAddingInvokableCreatesAliasAndMapsClassToElementFactory()
}
}

public function testAutoAddInvokableClass()
{
$instance = $this->manager->get(TestAsset\ConstructedElement::class, ['constructedKey' => 'constructedKey']);
Copy link
Member

Choose a reason for hiding this comment

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

Please reduce the line length to 80.


class ConstructedElement extends Element
{
public $constructedKey = null;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove null.

{
public $constructedKey = null;

public function __construct($name = null, $options = array())
Copy link
Member

Choose a reason for hiding this comment

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

Add a type hint for $options.

@turrsis
Copy link
Contributor Author

turrsis commented Apr 21, 2017

@froschdesign Your requested changes have been applied

@weierophinney weierophinney added this to the 2.10.1 milestone Apr 26, 2017
@weierophinney weierophinney self-assigned this Apr 26, 2017
@weierophinney weierophinney dismissed froschdesign’s stale review April 26, 2017 20:41

Author has made the requested changes.

weierophinney added a commit that referenced this pull request Apr 26, 2017
weierophinney added a commit that referenced this pull request Apr 26, 2017
Forward port #134

Conflicts:
	CHANGELOG.md
@weierophinney
Copy link
Member

Thanks, @turrsis

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

Successfully merging this pull request may close these issues.

4 participants