Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Iterator loop over the same item twice #73

Open
lekoala opened this issue Oct 24, 2023 · 4 comments · May be fixed by #74
Open

Iterator loop over the same item twice #73

lekoala opened this issue Oct 24, 2023 · 4 comments · May be fixed by #74
Labels

Comments

@lekoala
Copy link
Collaborator

lekoala commented Oct 24, 2023

Just found a really odd issue when testing sqlite

in a template i have this

<% loop MyList %>
...
<% end_loop %>

The list contains only one record, but it's displayed twice

It seems due to the switch to generators We have yield $data, but that is actually called twice due to the rewind call in SSViewer_scope.
I don't have a solution yet. Removing $this->itemIterator->rewind(); fixes the issue, but it's probably there for a reason.

Also, with sqlite, fetchArray can start giving AGAIN the results after false has been returned, so that might also be it, or a combination of both issues.

PRs

@lekoala
Copy link
Collaborator Author

lekoala commented Oct 24, 2023

so i think i got the general idea. The issue happens here

https://github.com/silverstripe/silverstripe-framework/blob/64e2b5e48981d68487a5be34b287018dcd6a482a/src/View/SSViewer_Scope.php#L303-L313

  • we get the iterator and the first row (yield)
  • we count it using numRecords in the DataList, but that reset the handle to the first row

https://github.com/silverstripe/silverstripe-framework/blob/64e2b5e48981d68487a5be34b287018dcd6a482a/src/ORM/DataList.php#L1502-L1509

  • the first row is iterated again

working on a fix :)

@lekoala lekoala linked a pull request Oct 24, 2023 that will close this issue
@lekoala
Copy link
Collaborator Author

lekoala commented Oct 24, 2023

so i have a solution
not sure it's the best one, but at least it's working :)

@GuySartorelli
Copy link
Member

This will need to wait until actions/runner-images#9033 is resolved so we can use a compatible version of sqlite3 in CI

@lekoala
Copy link
Collaborator Author

lekoala commented Dec 15, 2023

@GuySartorelli well, the ci is not needed to confirm that my fix works :-) but i'm in no rush, no issues with running my own little fork in the meantime

@GuySartorelli GuySartorelli removed their assignment Feb 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants