Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #3 - Element::init is called before Element::setOptions when using Factory but not when using FormElementManager #251

Draft
wants to merge 1 commit into
base: 3.17.x
Choose a base branch
from
Draft

Fixes #3 - Element::init is called before Element::setOptions when using Factory but not when using FormElementManager #251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/Factory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public function create($spec): ElementInterface
$spec = $this->validateSpecification($spec, __METHOD__);
$type = $spec['type'] ?? Element::class;

$element = $this->getFormElementManager()->get($type);
$element = $this->getFormElementManager()->get($type, $spec['options'] ?? []);
Copy link
Member

Choose a reason for hiding this comment

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

Build parameters must be ['options' => $spec['options']] according to ElementFactory and if options are not set or empty build parameters should be null to not trigger plugin manager's build() behavior.

ElementFactory also accepts element name as build parameters, so we should probably pass it too if $spec has it.

Something along those lines:

Suggested change
$element = $this->getFormElementManager()->get($type, $spec['options'] ?? []);
$buildParams = null;
if (! empty($spec['options'])) {
$buildParams = ['options' => $spec['options']];
$name = $spec['name'] ?? null;
if ($name !== null && $name !== '') {
$buildParams['name'] = $name;
}
}
$element = $this->getFormElementManager()->get($type, $buildParams);

Copy link
Member

Choose a reason for hiding this comment

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

note that options could be iterable, according to code. I did not handle it in suggested change.


if ($element instanceof FormInterface) {
return $this->configureForm($element, $spec);
Expand Down
17 changes: 17 additions & 0 deletions test/FactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,4 +849,21 @@
self::assertInstanceOf(TestAsset\FieldsetWithDependency::class, $targetElement);
self::assertInstanceOf(InputFilter::class, $targetElement->getDependency());
}

public function testSetOptionsShouldBeCalledBeforeInitMethodIsCalled(): void
{
$values = range(1, 10);

Check failure on line 855 in test/FactoryTest.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (PHPCodeSniffer [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-ac... 

Function range() should not be referenced via a fallback global name, but via a use statement.

/** @var TestAsset\SelectElementWithValueOptionsCheck $element */
$element = $this->factory->create(
[
'type' => TestAsset\SelectElementWithValueOptionsCheck::class,

Check failure on line 860 in test/FactoryTest.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (PHPCodeSniffer [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-ac... 

Expected 4 spaces before double arrow; 1 found
'options' => [
'value_options' => $values,
],
]
);

self::assertSame($values, $element->getValueOptions());
}
}
15 changes: 15 additions & 0 deletions test/FormElementManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,21 @@
self::assertEquals('bar', $element->getLabel(), 'Specified options in array[options]');
}

public function testSetOptionsShouldBeCalledBeforeInitMethodIsCalled(): void
{
$values = range(1, 10);

Check failure on line 126 in test/FormElementManagerTest.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (PHPCodeSniffer [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-ac... 

Function range() should not be referenced via a fallback global name, but via a use statement.

/** @var TestAsset\SelectElementWithValueOptionsCheck $element */
$element = $this->manager->get(
TestAsset\SelectElementWithValueOptionsCheck::class,
[
'value_options' => $values,
],
);

self::assertSame($values, $element->getValueOptions());
}

/**
* @group issue-6132
*/
Expand Down
17 changes: 17 additions & 0 deletions test/TestAsset/SelectElementWithValueOptionsCheck.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<?php

declare(strict_types=1);

namespace LaminasTest\Form\TestAsset;

use Laminas\Form\Element\Select;

use function assert;

final class SelectElementWithValueOptionsCheck extends Select
{
public function init(): void
{
assert($this->valueOptions !== []);
}
}
Loading