Repository navigation
feat!: fix three API defects that need a major version - #8
Merged
Merged
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
Three API defects that each need a major version, done together so there is one migration rather
than three — plus four bugs in the bulk-retrieval path that reviewing the first three exposed.
1. Collections returned
null. An empty Scopus result is routine, yetforeach ($results->getEntries() as $e)needed a null guard on the common path. Thirteen gettersnow return
[]and declare: array.2.
CitationCount::getStatus(): boolcould not answer its own question. The declaredboolcoerced Scopus's status string, so
"found"and"NOT_FOUND"both came back astrue— anynon-empty string did. It was also the only declared return type among forty-odd sibling getters,
and
ScopusApi::retrieve()has always compared the same field with=== 'found'. It now returnsthe raw string; the new
isFound(): boolanswers the boolean question correctly.3. Bulk retrieval hid every failure.
retrieveAbstracts()andretrieveAuthors()wrappedtheir single-id path in
catch (Exception $e) { return []; }, so a network failure, an invalidAPI key and a rate limit were indistinguishable from a document that was not found.
The bulk path was worse than it looked
Reviewing the above turned up four more defects in the same two methods. All are fixed here,
because the change already rewrites those lines:
The final chunk of one gets a single-document response rather than a list, so
array_combine()received an object:Argument #2 ($values) must be of type array, Scopus\Response\Abstracts given. This depends only on how many ids you pass, not on the data.array_merge()renumbers integer keys, soretrieveAbstracts([1, 2, 3])came back keyed0, 1, 2despite both methods promising a result keyed by id. String ids happened to survive,which is why it went unnoticed. Chunks are disjoint after
array_unique(), so the unionoperator is equivalent for string keys and correct for integer ones.
ValueErroron PHP 8, and — once this branch added: array— aTypeErroron 7.4 where it had silently returnednull. A new privatekeyable()helper now refuses to key what it cannot key, naming the counts and ids:Scopus returned 1 of the 2 requested documents (111,222), so results cannot be keyed by id.$ids[0]undefined; both methods now return[]without a request.retrieveAuthors()also deduplicated ids to choose its branch but chunked the originallist, so duplicates hit
array_combine()with mismatched counts.Testing
OK (33 tests, 133 assertions)— up from 23 — on every supported platform, dependenciesre-resolved per version:
--prefer-lowestsrc/lints clean on PHP 7.4, the declared floor. Ten new tests, including the chunk-of-one case(which is what caught the
array_mergekey bug), the partial-result refusal,NOT_FOUNDreporting
isFound()false, empty id lists making no request, and the six collection getters thathad no coverage for the changed contract.
Five existing assertions changed from
assertNulltoassertSame([]). That is the contractchange being asserted, not tests relaxed to pass — worth checking that judgement in review.
Notes
## [Unreleased]rather than cutting a version, sincereleases have been decided separately. The changelog carries an
### Upgradingsection.review found them. Worth adding before release.
if ($count->getStatus())are unaffected: a non-empty string is truthy, asbefore. Only
=== truecomparisons need moving toisFound().caches now use
??rather than?:, since a cached[]is falsy and was recomputed everycall; and
countAuthors()/countAffiliations()/countEntries()now count their own getter'sresult rather than the raw payload, so the two cannot disagree.
partial result returns what it found instead of throwing. That is a redesign of the return
contract, not a fix.
ScopusApi::retrieve()is still open and not bundled in.