Skip to content

feat!: fix three API defects that need a major version - #8

Merged
matasarei merged 10 commits into
masterfrom
feature/2.0-api-cleanup
Sep 3, 2026
Merged

matasarei merged 10 commits into
masterfrom
feature/2.0-api-cleanup

Conversation

@matasarei

@matasarei matasarei commented Sep 2, 2026 •

Copy link
Copy Markdown
Collaborator

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, yet
foreach ($results->getEntries() as $e) needed a null guard on the common path. Thirteen getters
now return [] and declare : array.

2. CitationCount::getStatus(): bool could not answer its own question. The declared bool
coerced Scopus's status string, so "found" and "NOT_FOUND" both came back as true — any
non-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 returns
the raw string; the new isFound(): bool answers the boolean question correctly.

3. Bulk retrieval hid every failure. retrieveAbstracts() and retrieveAuthors() wrapped
their single-id path in catch (Exception $e) { return []; }, so a network failure, an invalid
API 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:

  • Any request whose id count leaves a remainder of one — 26 ids, 51, 76 — died outright.
    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, so retrieveAbstracts([1, 2, 3]) came back keyed
    0, 1, 2 despite 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 union
    operator is equivalent for string keys and correct for integer ones.
  • A partial result from Scopus produced a ValueError on PHP 8, and — once this branch added
    : array — a TypeError on 7.4 where it had silently returned null. A new private
    keyable() 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.
  • Empty input reached $ids[0] undefined; both methods now return [] without a request.
    retrieveAuthors() also deduplicated ids to choose its branch but chunked the original
    list
    , so duplicates hit array_combine() with mismatched counts.

Testing

OK (33 tests, 133 assertions) — up from 23 — on every supported platform, dependencies
re-resolved per version:

PHP 7.4 8.0 8.1 8.4 8.5 7.4 --prefer-lowest
Result ✅ ✅ ✅ ✅ ✅ ✅ (guzzle 7.15.2)

src/ lints clean on PHP 7.4, the declared floor. Ten new tests, including the chunk-of-one case
(which is what caught the array_merge key bug), the partial-result refusal, NOT_FOUND
reporting isFound() false, empty id lists making no request, and the six collection getters that
had no coverage for the changed contract.

Five existing assertions changed from assertNull to assertSame([]). That is the contract
change being asserted, not tests relaxed to pass — worth checking that judgement in review.

Notes

  • This is 2.0.0 material. Left under ## [Unreleased] rather than cutting a version, since
    releases have been decided separately. The changelog carries an ### Upgrading section.
  • The changelog does not yet mention the four bulk-path fixes — it was written before the
    review found them. Worth adding before release.
  • Callers writing if ($count->getStatus()) are unaffected: a non-empty string is truthy, as
    before. Only === true comparisons need moving to isFound().
  • Two smaller cleanups ride along, both consequences of the collection change: the 30 lazy-init
    caches now use ?? rather than ?:, since a cached [] is falsy and was recomputed every
    call; and countAuthors()/countAffiliations()/countEntries() now count their own getter's
    result rather than the raw payload, so the two cannot disagree.
  • Deliberately not done: keying bulk results off each returned document's own identifier, so a
    partial result returns what it found instead of throwing. That is a redesign of the return
    contract, not a fix.
  • The API-key-header SSRF question in ScopusApi::retrieve() is still open and not bundled in.

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant