Add security advisory endpoint #21
Conversation
|
Seems good, please add tests |
GuySartorelli
force-pushed
the
pulls/main/add-advisories-endpoint
branch
from
02ea3e0 to
9b8d9e4
Compare
|
Now with tests (and fixed an unrelated one that was broken while I was at it). Please let me know if you need anything else from me. |
src/PackagistClient.php
Outdated
| @@ -84,6 +85,94 @@ public function getStatistics(): ?array | |||
| return $this->request('statistics.json'); | |||
| } | |||
|
|
|||
| /** | |||
| * Get security vulnerability advisories for specific packages and/or which have been updated since some timestamp. | |||
There was a problem hiding this comment.
We usually don't add such lengthy comments on how to use methods, but prefer methods that are obvious how you should use them.
We also don't like passing booleans to methods as this might hurt readability on the caller's side. Could you split this function into multiple other ones in which the use is clear?
There was a problem hiding this comment.
This is exactly the sort of thing I would have loved to be pointed out originally, when I hadn't yet written tests for the functionality as it's currently written. :p
I'll make the change, but really would have been nice to point this out when I was asking for exactly this sort of feedback originally.
There was a problem hiding this comment.
Split into two separate methods with no phpdocs
src/PackagistClient.php
Outdated
| return $advisories; | ||
| } | ||
|
|
||
| private function filterAdvisories(array $advisories, array $packages): array |
There was a problem hiding this comment.
We don't use private. Change this to protected.
src/PackagistClient.php
Outdated
| */ | ||
| public function getAdvisories(array $packages = [], ?int $updatedSince = null, bool $filterByVersion = false): array | ||
| { | ||
| if (count($packages) === 0 && $updatedSince === null) { |
There was a problem hiding this comment.
The readability of this code can be improved.
Please avoid using && and split this into multiple smaller functions so the comments can be removed.
There was a problem hiding this comment.
Inline comments removed.
That specific && is not longer required. I've simply removed the ability to get all packages by udpatedsince because this PR is becoming burdonsome to develop. If someone wants that functionality they can add it separately afterward.
GuySartorelli
force-pushed
the
pulls/main/add-advisories-endpoint
branch
from
11a5aff to
88c5697
Compare
GuySartorelli
force-pushed
the
pulls/main/add-advisories-endpoint
branch
from
88c5697 to
4095d3b
Compare
|
Thanks! |
Adds functionality to use the security advisories endpoint on the packagist api: https://packagist.org/apidoc#list-security-advisories
Draft because I want to know if this is an acceptable approach before I start adding tests.