Use a Generator for job list to fix background-job:list command#36073
Use a Generator for job list to fix background-job:list command#36073
Conversation
Signed-off-by: Côme Chilliet <[email protected]>
Signed-off-by: Côme Chilliet <[email protected]>
come-nc
added
3. to review
come-nc
requested review from
a team,
ArtificialOwl,
blizzz and
icewind1991
and removed request for
a team
|
|
… method Signed-off-by: Côme Chilliet <[email protected]>
come-nc
force-pushed
the
fix/fix-background-job-list
branch
from
c13c001 to
e74f464
Compare
Signed-off-by: Côme Chilliet <[email protected]>
|
Removed getAll and added getJobsIterator instead of changing the old one. |
| return $this->getJobs(null, null, 0); | ||
| public function getJobs($job, ?int $limit, int $offset): array { | ||
| $iterable = $this->getJobsIterator($job, $limit, $offset); | ||
| if (is_array($iterable)) { |
There was a problem hiding this comment.
when can an array be returned here?
There was a problem hiding this comment.
To avoid similar future API changes I did not type the return of getJobsIterator as Iterator but as iterable, which means array|Traversable.
Because of that, psalm consider that the value may be an array, even if in the current implementation it’s always a Generator.
|
Should be documented in #34692 |
nickvergessen
added
the
pending documentation
|
hello @come-nc |
come-nc
removed
the
pending documentation
6 participants
Signed-off-by: Côme Chilliet [email protected]
Summary
Job instances are singleton, so it is not possible to group them together in an array without getting corrupted data with several times the same id.
So instead I use a generator which is also better for memory consumption.
This breaks API for getJobs introduced in 25 by making it return a Generator instead of an array. Code calling can type that as iterable and only use foreach on it to be compatible with both 25 and 26.
This also breaks getAll as it calls getJobs, I would favor removing getAll because it is both broken and deprecated.
TODO
Checklist