API rescue master-branch PR: Use Generators for ORM #10484
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 tasks
|
CI failures are a result of #10389 |
GuySartorelli
force-pushed
the
pulls/5/rescue-master-orm-generators
branch
from
9cad48d to
746c831
Compare
|
Note: CI failures are for postgres (which will be resolved by silverstripe/silverstripe-postgresql#138) and PDO (which was deprecated as of this RFC so we're removing it anyway). |
Generators (PHP 5.5+) make this kind of code structure much easier to build.
Using a generator here means that we don’t need to prepare a duplicate array in-memory before iterating.
It wasn’t respecting pagination.
It’s best for foreach() to call this for us.
API: Query no longer has iterator methods current(), first(), rewind(), next() Using generators reduces the amount of boilerplate needed for this code. Turning it into an IteratorAggregate means that the iterator can be re-created for each subsequent foreach call. This means that the rewind() and seek() functionality can be discarded.
SSViewer iterates on Iterators that it receives twice: first to get the total number of items, then to actually render each item. This necessitates a rewind. In order to make more use of generators, which are not rewindable, I’d like to remove the need for a rewind. I’ve done this by caching the content of the iterator as an array within SSViewer_Scope. Although this means a bit of memory usage, there are no cases in which code will get to this point without iterating on all items, which would use the memory anyway. It would only create onerous impacts in cases where you are iterating on very long iterators, which would mean you’re rendering a very large page anyway, and probably have other performance issues.
`getIterator()` now returns a generator by default.
Setting a proper return type for these will be done in a separate PR
GuySartorelli
force-pushed
the
pulls/5/rescue-master-orm-generators
branch
from
3008b4e to
0f2ec49
Compare
GuySartorelli
force-pushed
the
pulls/5/rescue-master-orm-generators
branch
from
0f2ec49 to
e140c37
Compare
emteknetnz
approved these changes
Sep 15, 2022
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR rescues #6518 from the master branch. See the original PR for discussion about the implementation.
This PR was done once before but had to be reverted. See #10450 and #10483
The CI failures that were being caused have been resolved. The problem was that the PR being rescued was using old logic for iterators in the
MySQLQueryandMySQLStatementclasses. The correct logic is now in place - it was taken from thenextRecord()methods from those classes.Note that
yieldresults in returning aGeneratorwhich is a subclass ofIterator- so allgetIterator()methods affected now returnIterator- but I won't add the typehints for that in this PR as that's not directly the same concern as rescuing the master branch PR.NOTE
To avoid having to revert this again, don't merge until CI is green.
Parent Issue