[Cleanup] Make DBRowProvisioner use Database class #9509
Conversation
driusan
added
Cleanup
| @@ -1055,12 +1061,7 @@ class TimePoint implements \LORIS\StudyEntities\AccessibleResource, | |||
| ); | |||
|
|
|||
| $provisioner = $provisioner->filter($filter); | |||
|
|
|||
| $traversable = (new \LORIS\Data\Table()) | |||
There was a problem hiding this comment.
This part didn't seem to be useful, so I verified and removed while testing.
| @@ -83,50 +83,12 @@ abstract public function getInstance($row) : DataInstance; | |||
| */ | |||
| public function getAllInstances() : \Traversable | |||
| { | |||
| $DB = (\NDB_Factory::singleton())->database(); | |||
There was a problem hiding this comment.
This is the bulk of the PR. The rest is mostly just propagating LorisInstance objects places that didn't have it.
driusan
force-pushed
the
NoProvisionerStrVal
branch
from
3732d13 to
04ddf5c
Compare
| @@ -324,7 +324,7 @@ function testFilter() | |||
| self::$display, | |||
| self::$clearFilter, | |||
| '0', | |||
| '2 rows' | |||
There was a problem hiding this comment.
This test was failing and when I investigated I don't see any rows with a value of "0" in RB and nothing inserted from the test, so I'm not sure where "2" was coming from and I think it's an error.
There was a problem hiding this comment.
Can you change it to test '4300' with '1 rows' so that the test is still testing the filter? I feel like testing for no results is less foolproof, and the max age test covers that below anyway
There was a problem hiding this comment.
That's a good idea. I'll try.
driusan
force-pushed
the
NoProvisionerStrVal
branch
2 times, most recently
from
23420d6 to
4411abd
Compare
This is no longer required and causes obscure differences between pselect and DBRowProvisioner. Resolves aces#9335
driusan
force-pushed
the
NoProvisionerStrVal
branch
from
4411abd to
2f23574
Compare
| public function getDicomTars(\LORIS\LorisInstance $loris, \User $user): iterable | ||
| { | ||
| $provisioner = new \LORIS\api\provisioners\VisitDicomsRowProvisioner($this); | ||
| $traversable = (new \LORIS\Data\Table()) | ||
| ->withDataFrom($provisioner) | ||
| ->getRows($user); | ||
|
|
||
| return iterator_to_array($traversable); | ||
| $provisioner = new \LORIS\api\provisioners\VisitDicomsRowProvisioner( | ||
| $loris, | ||
| $this | ||
| ); | ||
| return iterator_to_array($provisioner->execute($user)); |
There was a problem hiding this comment.
This was changed to return type iterator but returns iterator_to_array, so isn't return type still an array?
There was a problem hiding this comment.
You are right. I originally didn't have the iterator_to_array and it was working but then I had to include it to get the phan (or phpstan, can't remember) tests to pass. It's probably kind of pointless now.
| @@ -324,7 +324,7 @@ function testFilter() | |||
| self::$display, | |||
| self::$clearFilter, | |||
| '0', | |||
| '2 rows' | |||
There was a problem hiding this comment.
Can you change it to test '4300' with '1 rows' so that the test is still testing the filter? I feel like testing for no results is less foolproof, and the max age test covers that below anyway
skarya22
added
the
State: Needs rebase
This updates DBRowProvisioner to remove the strval (and fixes #9335).
After doing that, it became clear that the inner class could be replaced by a generator while still fulfilling the contract of returning a
\Traversable. Doing this made it more obvious that since #9334, pselect could be used directly while maintaining the lazy evaluation instead of bypassing the Database object to use the PDO directly. However this required injecting aLorisInstanceobject to the constructor to get the database connection.With access to the
LorisInstanceobject, theDBRowProvisionercan also get a new database connection and disable query buffering to lazily retrieve rows since PR #9344. This may theoretically improve performance and memory usage on large provisioners but has not been tested. (Performance did not decrease in raisinbread but it is not a large dataset where it would be noticeable.)