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

Fixes #6768 : boolean false values at priority list should be valid #6773

Closed

Conversation

samsonasik
Copy link
Contributor

Fixes #6768

@samsonasik samsonasik changed the title Fixes #6768 : boolean values at priority list should be valid Fixes #6768 : boolean false values at priority list should be valid Oct 16, 2014
@bgaillard
Copy link

Hi, this is a critical issue, it prevents us to upgrade from zf 2.2 to zf 2.3.

As their are unit tests is it possible to quickly integrate this fix in the next minor version of the framework ?

Thanks.

@@ -227,7 +235,8 @@ public function next()
*/
public function valid()
{
return ($this->current() !== false);
Copy link
Contributor

Choose a reason for hiding this comment

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

enough return (current($this->items) !== false);
protected $currentNode = false; is superfluous

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 ;). thank you ;)

@samsonasik samsonasik force-pushed the hotfix/prioritylist branch 2 times, most recently from faf7f7b to 5b4443c Compare November 19, 2014 13:00
*/
public function testBooleanValuesAreValid()
{
$this->list->insert('foo', false, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

please, add $this->list->insert('foo', null, null); here

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 ;)

/**
* @group ZF2-6768
*/
public function testBooleanValuesAreValid()
Copy link
Contributor

Choose a reason for hiding this comment

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

was meant this:

        $this->list->insert('null', null, null);
        $this->list->insert('false', false, null);
        $this->list->insert('string', 'test', 1);
        $this->list->insert('true', true, -1);

        $orders1 = array();
        $orders2 = array();

        foreach ($this->list as $key => $value) {
            $orders1[$this->list->key()] = $this->list->current();
            $orders2[$key] = $value;
        }
        $this->assertEquals($orders1, $orders2);
        $this->assertEquals(
            array(
                'null'   => null,
                'false'  => false,
                'string' => 'test',
                'true'   => true,
            ),
            $orders2
        );

@turrsis
Copy link
Contributor

turrsis commented Nov 20, 2014

@Ocramius this is realy critical bug

@Ocramius Ocramius self-assigned this Nov 20, 2014
@Ocramius Ocramius added this to the 2.3.4 milestone Nov 20, 2014
Ocramius added a commit that referenced this pull request Nov 20, 2014
… into its own test method with `@group` annotations
Ocramius added a commit that referenced this pull request Nov 20, 2014
Ocramius added a commit that referenced this pull request Nov 20, 2014
Ocramius added a commit that referenced this pull request Nov 20, 2014
Ocramius added a commit that referenced this pull request Nov 20, 2014
…'develop'

Close #6773
Close #6768
Close #6820
Close #6834
Close #6881
Forward port #6773
Forward port #6768
@Ocramius Ocramius closed this in 2add207 Nov 20, 2014
@Ocramius
Copy link
Member

Merged, thanks!

master: 2add207
develop: c2e67b8

@samsonasik
Copy link
Contributor Author

@Ocramius test case on develop failure, it seems some commits in master in valid() not merged to develop, no ?

@Ocramius
Copy link
Member

@samsonasik thanks for noticing. This is indeed a merge conflict SNAFU. Will fix right now.

@Ocramius
Copy link
Member

@samsonasik interestingly, Stdlib tests pass, whereas the Db tests fail. Maybe the test case doesn't cover everything accurately? Can you look into it?

@samsonasik
Copy link
Contributor Author

@Ocramius latest master :
return current($this->items) !== false;
latest develop :
return $this->current() !== false;

It seems I need to rollback to use "hold" currentNode like this : samsonasik@e9812a0 ?

@Ocramius
Copy link
Member

@samsonasik besides that, the unit test in ZendTest\Stdlib doesn't expose the failure, that's why I didn't notice the regression.

@turrsis
Copy link
Contributor

turrsis commented Nov 20, 2014

@samsonasik I try to use all your tests without $currentNode and with return current($this->items) !== false; - all was ok

Ocramius added a commit that referenced this pull request Nov 20, 2014
…ort-check' into hotfix/#6904-#6773-zend-stdlib-priority-list-false-value-support

Close #6904
Ocramius added a commit that referenced this pull request Nov 20, 2014
…ll` values in the `PriorityList`
Ocramius added a commit that referenced this pull request Nov 20, 2014
@turrsis
Copy link
Contributor

turrsis commented Nov 20, 2014

@samsonasik
Copy link
Contributor Author

@turrsis should already been updated at #6904

@turrsis
Copy link
Contributor

turrsis commented Nov 20, 2014

@Ocramius @samsonasik sorry - all ok

gianarb pushed a commit to zendframework/zend-stdlib that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-stdlib that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-stdlib that referenced this pull request May 15, 2015
…emoving redundant docblocks, cleaning up comparison operations syntax and simplifying code
gianarb pushed a commit to zendframework/zend-stdlib that referenced this pull request May 15, 2015
…valid()` should return `true` also for `false` and `null` values in the `PriorityList`
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.

Zend\Stdlib\PriorityList cannot contain false values
5 participants