laminas / laminas-db

Database abstraction layer, SQL abstraction, result set abstraction, and RowDataGateway and TableDataGateway implementations
https://docs.laminas.dev/laminas-db/
BSD 3-Clause "New" or "Revised" License
122 stars 68 forks source link

Fix data loss on multiple `Result#rewind()` calls #273

Closed Grundik closed 1 year ago

Grundik commented 1 year ago
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.

Grundik commented 1 year ago

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 commented 1 year ago

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

Ocramius commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

That's on class implements AbstractResultSet in general

https://github.com/laminas/laminas-db/blob/eff7c7dd52d2e809540ca89c5935d8832893b29e/src/ResultSet/AbstractResultSet.php#L97-L108

samsonasik commented 1 year ago

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 commented 1 year ago

Will this be enough?

samsonasik commented 1 year ago

Yes, that's should be enough 👍

Grundik commented 1 year ago

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

samsonasik commented 1 year ago

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

Ocramius commented 1 year ago

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