New: Add security advisories endpoint. #86
Conversation
|
Thanks for the PR. This looks like a good new feature to me! |
GuySartorelli
force-pushed
the
pulls/master/add-advisory-endpoint
branch
from
66b2bcf to
92e4513
Compare
|
Have added tests. Please let me know if you need anything else. I'm not familiar with the style of tests used in this package so I don't know if there's an established way to do end-to-end tests for example. |
|
The BDD tests are unit tests, an E2E test is usually done manually using the example scripts in this repository. Thanks very much for the contribution, great work. I'll review it next week. |
|
By the way @GuySartorelli I've approved the CI workflow to run, there are a couple of failures |
GuySartorelli
force-pushed
the
pulls/master/add-advisory-endpoint
branch
from
92e4513 to
33e65a7
Compare
|
I wasn't sure how to run the tests locally so I was just kind of hoping it would pass... have now learned how to run them and have updated them. They pass locally now. |
There was a problem hiding this comment.
This looks good, I think if you could update the doc blocks to be a little more descriptive that'd be great. I'd also suggest adding an example file, i.e.:
# File: examples/advisories.php
<?php
require __DIR__ . '/../vendor/autoload.php';
$client = new Packagist\Api\Client();
$package = $client->advisories(['monolog/monolog']);
var_export($package);| @@ -187,6 +189,88 @@ public function popular(int $total): array | |||
| return array_slice($results, 0, $total); | |||
| } | |||
|
|
|||
| /** | |||
| * Get a list of known security vulnerability advisories | |||
There was a problem hiding this comment.
I think this docblock would benefit from some extra information describing what the $updatedSince and $filterByVersion arguments are for. It's not immediately obvious to me.
There was a problem hiding this comment.
I've updated the phpdoc to be more descriptive
| /** | ||
| * Get a list of known security vulnerability advisories | ||
| * | ||
| * @param array $packages |
There was a problem hiding this comment.
My initial instinct when testing this was to call $client->advisories('monolog/monolog'). I think it's fine to enforce an array of package names here (maybe add this as a description on this argument). I then thought it might be nice to be able to access advisories from the Package class, i.e. $package->advisories(), but the Package class doesn't have client awareness so it would require making the AbstractResult classes aware of the client. I don't think it's worth it, but it would be nice to have that accessor method.
(Not asking for changes here, just voicing my thoughts while testing)
There was a problem hiding this comment.
Yeah I thought about allowing Package values in the array, but all of the other methods that accept a package as an argument only accept the string names, so I figured it made sense to be consistent and do the same here.
Maybe a future enhancement could be to change all of those to accept Package|string.
GuySartorelli
force-pushed
the
pulls/master/add-advisory-endpoint
branch
from
33e65a7 to
b9f74e2
Compare
|
Requested changes made. I've left the phpdoc for the results either missing or sparse because that seems to be the standard for result classes but let me know if you'd like those to have some explanation as well... but generally I'd expect anyone who wants to use this to already have checked out the packagist docs which show exactly what sort of result the api returns. |
|
@GuySartorelli I've included this in v2.0.0-rc3. If this release looks good to you I'll tag v2.0.0. Thanks for the PR! |
|
Awesome, thank you. I'll try to give the release a proper look on the weekend. Looks good at a cursory glance though |
|
@robbieaverill Sorry it's taken me so long to get back to you - the RC looks great! |
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.