Add getAllStatements method to Statement Response

[garemoko]

Something like this:

$allTheStatements = $statementsResponse->content->getStatements();
    $moreStatementsURL = $statementsResponse->content->getMore();
    while (!is_null($moreStatementsURL)) {
        $moreStmtsResponse = $this->moreStatements($moreStatementsURL);
        $moreStatements = $moreStmtsResponse->content->getStatements();
        $moreStatementsURL = $moreStmtsResponse->content->getMore();
        //Note: due to the structure of the arrays, array_merge does not work as expected.
        foreach ($moreStatements as $moreStatement) {
            array_push($allTheStatements, $moreStatement);
        }
    }

[brianjmiller]

My concern with including something like this in the library is that it is blocking and could block for a really long time and cause an inordinate number of requests to the LRS. If we add it in the sync case we need a way to make it cancellable and I'm not sure how we'd implement that.

[garemoko]

I'm not sure if this is the best way, but one way to make it cancelable would be to have some sort of global flag that gets checked each time it loops.

[brianjmiller]

The problem with that approach would be that you can't be doing work to set the global because you've been blocked.

[garemoko]

What about a timeout and/or max number of requests parameter?

[brianjmiller]

Sure, but which, both? Ultimately these questions are all excellent, but also the reason why we just leave it up to the user to decide what works best for their specific use case :-). The basic implementation is incredibly simple, as you've shown, the one that is generic enough to be useful gets complex really quickly and ultimately probably only saves 8 lines of code. Possibly the best thing to do is just to add it to the documentation with a warning about the blocking nature and that people should implement their own sort of checks. For anyone that finds the above code, it does need error handling :-).

[WillSkates]

Maybe using Generators?

$all = $statementsResponse->content->getStatements();
yield $all;
while($more = $statementsResponse->content->getMore()) {
    $batch = $this->fetchStatements($more);
    foreach ($batch as $statement) {
        $all[] = $statement;
    }
    yield $all;
}
?>

That way the calling code can itterate over the result and decide when to stop.

[brianjmiller]

@WillSkates interesting. With us dropping 5.4 this can be an option. Would need to be named appropriately given the above implementation, though I'd wonder does it make more sense to yield after each statement, and/or provide both interfaces? Do people care about getting batches of statements?

[WillSkates]

@brianjmiller I'd avoid doing single statements as you would have to make a single request for each one.

$statements = $lrs->findStatements(40);
$allStatements = $lrs->findAllStatements();

Is closer to what you want right? In either case the RemoteRLS class should have some sensible defaults as to how many statements it will attempt to get in a single request. If you need/want to override those defaults then maybe something like these:

$operation = new RemoteLRS\Operations\FindSomeStatements(40);
$operation  = $operation->withBatchSize(5);
$result = $lrs->performOperation($operation);
$statements = $result->statements->all();

$operation = new RemoteLRS\Operations\FindAllStatements();
$operation  = $operation->withBatchSize(33);
$result = $lrs->performOperation($operation);
$statements = $result->statements->all();

[brianjmiller]

@WillSkates no, I meant yielding after each statement which should be a fairly tight loop. Batch sizing is already handled via the limit query string parameter which is already in that interface and handled at the LRS level. So you would request a block of statements (with or without specifying a limit which is really just page size) and the yield would occur after each statement until you stopped the loop or you ran out of statements.

The bottom interface(s) is exactly why I don't want to take on the burden of trying to determine all the necessary options in this library and feel like it should be handled one level up. Though with the generators we can make it easier to implement one level up.

No matter what we do I want to avoid using the word "find" in the method name, cause that confuses people and this isn't a querying interface.

[WillSkates]

@brianjmiller Yielding after each statement would probably be fine. Something like this maybe:

public function fetchAllStatements($batchSize = 10)
{
    $current = 0;
    $batch = [];
    while ($batch) {
           if (count($batch) == 0) {
                 $batch = $this->fetchStatementBatch($current * $batchSize, $batchSize);
                 if ($batch === false) {
                     break;
                 }
                 $current++;
           }
           yield array_shift($batch);
    }
}