Fix data loss on multiple Result#rewind() calls

[Grundik]

Q	A
Documentation	no
Bugfix	yes
BC Break	maybe?
New Feature	no
RFC	no
QA	no

Description

Fixes #196.

BC break can occur if one deliberately used this bug as a feature. But that should not be the case in sane projects.

[Ocramius]

This looks good to me, although I don't understand the code 🤣

To be clear, previously rewinding twice would lead to undefined behaviour?

[Grundik]

Yes, as stated in referenced issue. Calling ->rewind() just caused one row to be read from database. Always. Even if current positions already was at the beginning of result.

[samsonasik]

@Ocramius iirc, call ->rewind() was requires ->buffer() call to always back to position 0. It probably requires new test on combination with ->buffer()

[Ocramius]

Ok, given @samsonasik's feedback, could such a test be introduced, @Grundik?

I'd then gladly merge, even at the risk of minor BC issues.

[Grundik]

As I can see, buffering for PDO results are explicitly disabled:

    /**
     * @return void
     */
    public function buffer()
    {
    }

    /**
     * @return bool|null
     */
    public function isBuffered()
    {
        return false;
    }

So, this should not be an issue at all.

[samsonasik]

That's on class implements AbstractResultSet in general

laminas-db/src/ResultSet/AbstractResultSet.php

Lines 97 to 108 in eff7c7d

	public function buffer()
	{
	if ($this->buffer === -2) {
	throw new Exception\RuntimeException('Buffering must be enabled before iteration is started');
	} elseif ($this->buffer === null) {
	$this->buffer = [];
	if ($this->dataSource instanceof ResultInterface) {
	$this->dataSource->rewind();
	}
	}
	return $this;
	}

[samsonasik]

This is my old blog post about buffer() to set position 0 on rewind() call

https://samsonasik.wordpress.com/2019/08/05/using-buffer-for-resultsetrewind-after-resultsetnext-called-in-zenddb/

[Grundik]

Will this be enough?

[samsonasik]

Yes, that's should be enough 👍

[Grundik]

Sorry! Extra code have slipped through. Removed duplicate of existing test.

[samsonasik]

I think this should go to 2.17.0 since it behaviour change, I will let's @Ocramius decide on that

[Ocramius]

@samsonasik as a bugfix, targeting for a patch release: bugfixes are behaviour changes anyway 😁

[Ocramius]

Thanks @Grundik!