IBX-143: Refactored reindexing command to be DBAL 2.13-compatible #3094
Conversation
Co-Authored-By: Adam Wójs <adamwojs@users.noreply.github.com>
|
Update: I've reworked IndexerGateway to encapsulate all database operation, exposing only ready to consume generators. Added new namespace for SPI gateway integration test. Tests are re-using legacy Repository setup schema & data due to time constraints. Waiting for Travis... |
| do { | ||
| $contentIds = []; | ||
| for ($i = 0; $i < $iterationCount; ++$i) { | ||
| if ($contentId = $stmt->fetchOne()) { | ||
| $contentIds[] = $contentId; | ||
| } elseif (empty($contentIds)) { | ||
| return; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| yield $contentIds; | ||
| } while (!empty($contentId)); |
There was a problem hiding this comment.
I have feeling that something is wrong here but maybe it's lack of coffee...
There was a problem hiding this comment.
Lack of coffee 😄
I was also bewildered by this code though, and for this reason wanted phpdoc to include the fact that the generator built here will actually return arrays of integers (int[]), not single integers (int). But PHPStorm refused to understand the @return \Generator<int[]> annotation 😝
There was a problem hiding this comment.
I have feeling that something is wrong here but maybe it's lack of coffee...
It's a multidimensional array - a list of Content IDs for each iteration.
There was a problem hiding this comment.
Variable names are misleading, $contentId as an array of IDs, and $contentIds as an array of arrays of IDs bring a lot of confusion to this code.
There was a problem hiding this comment.
@Nattfarinn actually no. $contentId is a single ID indeed.
To resolve the readability issue, I suggest we move the internal iteration to a separate, private method that will just build that $contentIds chunk?
Co-authored-by: Adam Wójs <adam@wojs.pl>
v2.5,v3.2,v3.3Since eZ Platform 2.5 requires Doctrine Bundle ^1.9 which requires
doctrine/dbal:^2.5.12, it's safe to forcedoctrine/dbal:^2.13, which introduces some changes ResultStatement-related class structure.This PR besides fixing the issue at hand, refactors the code to be more future-proof and adds test coverage to prevent further regressions.
Checklist:
$ composer fix-cs).@ibexa/engineering).