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

fixes #6585 #6614

Closed
wants to merge 13 commits into from
Closed

fixes #6585 #6614

wants to merge 13 commits into from

Conversation

vixriihi
Copy link
Contributor

fixes #6585

@Martin-P
Copy link
Contributor

Martin-P commented Oct 3, 2014

It fixes the issue, but isn't it better to create a method that does one thing and only populates? The values can be provided as an argument to that method like this:

$data = $this->extract();
$this->populate($data);

That looks a bit cleaner to me instead of one call to $this->extractAndPopulate();.

@vixriihi
Copy link
Contributor Author

vixriihi commented Oct 3, 2014

Agree that would make more elegant solution. Made the changes

* @param bool $onlyBase
* @return void
*/
public function populateValues($data, $onlyBase = false)
Copy link
Member

Choose a reason for hiding this comment

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

Should be private or protected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's overriding method from fieldset. So either we need to figure out a new name for this or keep it public :(
We could take that method away from the form all together but that would change the existing behavior a little (Namely that the form would populate every field instead of just the base during bind and validation without data)..Tests seem to pass then also, but I'd be surprised if there wouldn't be any side effects in some app.

Copy link
Member

Choose a reason for hiding this comment

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

If it's an override then it's fine: I just don't want new API that is an implementation detail :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me neither :)

Copy link
Member

Choose a reason for hiding this comment

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

If it's inherited, could you replace the docblock with {@inheritDoc}?

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

@@ -522,4 +523,25 @@ public function testShouldValidateAllowObjectBindingByObject()
$this->assertTrue($allowed);
}

public function testBindValuesPreservesNewValueAfterValidation()
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you can add a group annotation so it is clear why the test is needed? See #6711

@Ocramius
Copy link
Member

Ocramius commented Dec 2, 2014

Linking #6711

@Ocramius Ocramius added this to the 2.3.4 milestone Dec 2, 2014
@Ocramius Ocramius self-assigned this Dec 2, 2014
Ocramius added a commit that referenced this pull request Dec 5, 2014
@Ocramius Ocramius closed this in fa6efc3 Dec 5, 2014
Ocramius added a commit that referenced this pull request Dec 5, 2014
…g-form-elements' into develop

Close #6585
Close #6614
Forward port #6585
Forward port #6614
@Ocramius
Copy link
Member

Ocramius commented Dec 5, 2014

Manually rebased/merged, thanks!

master: fa6efc3
develop: 2f5e624

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.

Form Collection No element by the name of [2] found in form
3 participants