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

Updated Form class, bindValues function to set to empty array if no … #216

Merged
merged 5 commits into from
Dec 11, 2018

Conversation

rkeet
Copy link
Contributor

@rkeet rkeet commented Jul 22, 2018

Updated Form class, bindValues function to set to empty array if no data to pass along for baseFieldset

Provide a narrative description of what you are trying to accomplish:

  • Are you fixing a bug?

In detail: #215

In short:

Form->bindValues($data, ...) calls Fieldset->bindValues($data, ...).

Fieldset has the function defined as:

public function bindValues(array $values = [], array $validationGroup = null)

The $data var was set with the following line:

$data = $data[$this->baseFieldset->getName()];

In the case of no defined Input elements, this may cause there to be no key with that name and have the $data variable be null. This then causes a fatal error when Form->bindValues($data, ...) calls Fieldset->bindValues($data, ...)


In respect, I've assumed that no tests need to be modified (they pass btw, though there's 40'ish that don't run because they're for Annotation) as this function was assumed to work with the happy-flow of properly calling the Fieldset->bindValues() function. This change just catches a bug but does not modify the intended functionality (I think).

@froschdesign
Copy link
Member

@rkeet

In respect, I've assumed that no tests need to be modified

But we need an unit test to cover your scenario.

@rkeet
Copy link
Contributor Author

rkeet commented Jul 22, 2018

Created a test case for the scenario. Am tired though, so please check it as well. More another day.

$form->add($baseFieldset);
$form->setHydrator(new ObjectPropertyHydrator());

$baseFieldsetInputFilter = new InputFilter();
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 remove all these lines related to the input-filters. (830 - 835)
The result is the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Did indeed not make a difference for patch.

$baseFieldset = new Fieldset('base_fieldset');
$baseFieldset->setUseAsBaseFieldset(true);

$form = new Form('default_form');
Copy link
Member

Choose a reason for hiding this comment

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

An unit test should be reduced to a minimum. The name for the form is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


$form = new Form();
$form->add($baseFieldset);
$form->setHydrator(new ObjectPropertyHydrator());
Copy link
Member

Choose a reason for hiding this comment

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

Please use $form->setHydrator(new Hydrator\ObjectProperty());

Copy link
Member

Choose a reason for hiding this comment

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

Both are imported, and there are examples of each in the test case. #224 resolves the consistency issues, so I'll leave as-is for now.

@weierophinney weierophinney added this to the 2.12.1 milestone Dec 11, 2018
@weierophinney weierophinney merged commit a3edaaf into zendframework:master Dec 11, 2018
weierophinney added a commit that referenced this pull request Dec 11, 2018
Updated Form class, bindValues function to set  to empty array if no …
weierophinney added a commit that referenced this pull request Dec 11, 2018
weierophinney added a commit that referenced this pull request Dec 11, 2018
weierophinney added a commit that referenced this pull request Dec 11, 2018
@weierophinney
Copy link
Member

Thanks, @rkeet!

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.

3 participants