[regression] fetchAllAssoc() and fetchAllKeyed() behave inconsistently

A test-only patch to demonstrate the issue.

The last submitted patch, 2: 3350375-2-test-only.patch, failed testing. View results →

Fix would be avoiding to use foreach.

Ran the tests without the fix locally and they passed unfortunately.

@smustgrave on which db type did you test?

Tested on fresh installs of mariaDb, mysql, and postgres and the tests pass each time without the fix.

Won't move to NW though because clearly they are failing here.

The last submitted patch, 2: 3350375-2-test-only.patch, failed testing. View results →

Just rerun tests in #2, and mysql and pgsql fail as expected - sqlite is not failing, expected as well, since that driver is not using StatementWrapperIterator.

Will try on github, in the meantime NR by anyone else that can reproduce @smustgrave results and figure out why.

I setup a Github Action test workflow - https://github.com/mondrake/d8-unit/blob/test-3350375/.github/workflows/...

and I get the test failures - https://github.com/mondrake/d8-unit/actions/runs/4630185826

So I cannot replicate #6 and #8

I won't be the hold up as my local could be different and change isn't so large I must get a failure. Will let the committers decide on this one but don't mind marking as #2 shows the tests do fail (reran earlier).

Whilst this seems logical to me, I have a concern that this may disrupt someone who has written existing code that relies on the current behaviour.

Is this something we can make opt-in (e.g. via settings.php flag) that we default to on for new installs?

Would like a release manager to have a look at this and assess the possible risk of disruption, so tagging as such.

Can we get a change record here too, will make it easier for a release manager to review as well.

It seems unlikely that someone would start fetching individual records and then call fetchAll() half way through... but also you never know.

Is there some way we can detect that iteration has already begun? But then I guess the new code allows for this, just with different results, so it's not really a deprecation as such, just a behaviour change.

Overall I feel like this is probably OK in a minor release with a change record and release note, although it might be better to commit early in 10.2 to give the maximum time for anyone to run into issues (which won't be long now before 10.2 is open really).

@mondrake what was the behaviour prior to 🐛 Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding Fixed ?

Answering my own question. This is a regression introduced by 🐛 Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding Fixed and should be fixed prior to 10.1.x being released.

I reverted c6532d8c5c the commit hash for 3265086 and added @mondrake's new tests. They pass - whereas on head they fail. This is for both mysql and sqlite (so both flavours of statement class we have).

@mondrake can you confirm my findings? If this is the case I don't think we need a change record or a release manager review.

This is an edge case, I do not think anyone was actually starting to fetch single records to then fetchAll* the remaining part. I am not sure what the situation was before 🐛 Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding Fixed ; I will try a test-only patch run on 10.0 and 9.5 to discover (I tried running the one in #2 but that does not apply).

I also think we should consider this a completion of 🐛 Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding Fixed .

Here's proof that this is a regression on 10.1.x - here's the test-only patch but for 10.0.x.

The postgres fails on 10.0.x are due to a regression caused by 🐛 Schema::changeField() has bug when changing regular serial field to big serial field Fixed that's been fixed due to a revert. As #18 shows us this fixes a regression caused by 🐛 Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding Fixed therefore this does not need a CR or release manager review as we're fixing an accidental behaviour change in 10.1.x and the original RTBC in #12can stand.

Committed 1057a39 and pushed to 10.1.x. Thanks!

mondrake → closed merge request !3729

Automatically closed - issue fixed for 2 weeks with no activity.