Improved previous fix for ZF2-558. #2438

Closed
wants to merge 10 commits into
from

Projects

None yet

3 participants

@timdev
timdev commented Sep 27, 2012

Previous fix made implicit assumptions that could cause issues in subclasses of Element[Select|MultiCheckbox]. This fix dispenses with those assumptions so subclasses that override getValidator() and attach validators that are neither InArray or Explode won't break. For instance, DoctrineORMModule\Form\Element\DoctrineEntity was broken by the original fix (294a7d1)

Tim Lieberman Improved previous fix for ZF2-558. Previous fix made implicit assumpt…
…ions that could cause issues in subclasses of Element\[Select|MultiCheckbox]
d5f24fc
@bakura10
Contributor

Speaking about DoctrineEntity element, I made a PR to fix the error (but it has not been merged yet).

Anyway I think you're right, it was wrong to assume the validator was always InArray.

@weierophinney weierophinney commented on an outdated diff Oct 1, 2012
library/Zend/Form/Element/MultiCheckbox.php
@@ -64,7 +64,7 @@ public function setValueOptions(array $options)
$this->valueOptions = $options;
// Update InArray validator haystack
- if (!is_null($this->validator)) {
+ if (!is_null($this->validator) && $this->validator instanceof ExplodeValidator) {
@weierophinney
weierophinney Oct 1, 2012 Member

Simply doing the instanceof check would suffice here:

if ($this->validator instanceof ExplodeValidator) {
@weierophinney
weierophinney Oct 1, 2012 Member

Also, update the preceding comment, as it's now invalid.

@weierophinney weierophinney and 1 other commented on an outdated diff Oct 1, 2012
library/Zend/Form/Element/Select.php
@@ -83,8 +83,15 @@ public function setValueOptions(array $options)
// Update InArrayValidator validator haystack
if (!is_null($this->validator)) {
- $validator = $this->validator instanceof InArrayValidator ? $this->validator : $this->validator->getValidator();
- $validator->setHaystack($this->getValueOptionsValues());
+ if ($this->validator instanceof InArrayValidator){
+ $validator = $this->validator;
+ }
+ if ($this->validator instanceof ExplodeValidator){
+ $validator = $this->validator->getValidator();
+ }
+ if (!empty($validator)){
@weierophinney
weierophinney Oct 1, 2012 Member

This should likely check the validator type; otherwise, it's possible that the next line would call an undefined method if the validator type is not what is expected.

@timdev
timdev Oct 1, 2012

Not possible, since if $this->validator is anything other than InArray or Explode, $validator will be empty. Or am I missing something?

@timdev
timdev Oct 1, 2012

Oh, no, you're right. You can set Explode's composed validator to anything.

@weierophinney weierophinney was assigned Oct 1, 2012
@weierophinney weierophinney added a commit that referenced this pull request Oct 2, 2012
@weierophinney weierophinney [#2438] CS fixes
- Trailing whitespace
be4f91d
@weierophinney weierophinney added a commit that referenced this pull request Oct 2, 2012
@weierophinney weierophinney Merge branch 'hotfix/2438' into develop
Forward port #2438
b243c1c
@weierophinney weierophinney added a commit that closed this pull request Oct 2, 2012
@weierophinney weierophinney Merge branch 'hotfix/2438'
Close #2438
1d1ce51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment