Skip to content

feat(context): get()/getLocal() throw on a missing key instead of returning null#195

Merged
EdmondDantes merged 3 commits into
mainfrom
context-get-must-throw
Jul 15, 2026
Merged

feat(context): get()/getLocal() throw on a missing key instead of returning null#195
EdmondDantes merged 3 commits into
mainfrom
context-get-must-throw

Conversation

@EdmondDantes

Copy link
Copy Markdown
Contributor

The problem

get() was an exact duplicate of find(), and getLocal() of findLocal(). Four methods, two behaviours:

find(parentKey)      'P'      get(parentKey)       'P'
findLocal(parentKey) NULL     getLocal(parentKey)  NULL
                              get(missing)         NULL

Both walked the parent Scopes; both answered null on a miss. Half of the API had no reason to exist.

And three sources described three different things:

Source Claim
The site docs "Unlike find(), this method throws if the key is not found at any level. Use get() when the value is mandatory." — and @throws Async\ContextException
The stub "Get a value by key in the current Context" — i.e. no parent lookup
The code Identical to find(). Returns null. ContextException does not exist.

The site was describing the useful contract — the standard find / get split (cf. PSR-11) — and the code simply never implemented it.

The change

  • find() / findLocal() — the optional-value forms. Unchanged: they answer null.
  • get() / getLocal() — the mandatory-value forms. A missing key now raises Async\ContextException, the class the docs already referred to.
$context->find('missing');      // null
$context->get('missing');       // Async\ContextException: Context key "missing" not found
$context->get($objectKey);      // Async\ContextException: Context key of type Foo not found

Parent lookup is untouched — get() still reaches inherited Scopes, exactly like find(). Only the miss differs.

Registration order

async_register_context_ce() ran before async_register_exceptions_ce(), so ContextException was handed a NULL parent and tripped Exceptions must implement Throwable. Context registration now follows the exceptions, as Channel's already does.

BC break — deliberate, and two tests updated rather than worked around

  • 008-context_get_missing.phpt was literally named "Context: get() returns null for missing keys". It pinned the behaviour we are calling a bug. It now covers the throw, and keeps a find() call so the null half of the contract stays pinned.
  • 002-context_inheritance.phpt used getLocal() to show that a local lookup does not reach the parent. That intent is unchanged — under the new contract it is expressed by the throw rather than by a null. findLocal() still pins the null.

Tests

  • New 011get()/getLocal() throw, find()/findLocal() answer null, present keys still return, ContextException extends AsyncException.
  • New 012get() reaches parent Scopes; getLocal() does not; object keys are reported by class name.
  • context 12/12. Full ext/async/tests: no new failures — the 3 that fail (curl/063, curl/064, io/082) fail identically on a pristine baseline.

The 36 mentions of ContextException in the docs were left alone on purpose while this was pending; they are now correct.

…urning null

get() was an exact duplicate of find(): both walked the parent Scopes and both answered
null when the key was absent. getLocal() and findLocal() were the same pair. Four methods,
two behaviours -- so half of them had no reason to exist.

Three sources described three different things. The site said get() throws ContextException
when the key is missing; the stub said get() searches only the current Context; the code did
neither. The site was describing the useful contract, and it is the one adopted here: find()
is the optional-value form and answers null, get() is the mandatory-value form and raises
Async\ContextException. The class the docs already referred to now exists.

Registration had to move: async_register_context_ce() ran before
async_register_exceptions_ce(), so ContextException would have been handed a NULL parent and
tripped the "Exceptions must implement Throwable" assertion.

BC break, deliberate. Two existing tests pinned the old behaviour and are updated rather than
worked around:

  * 008 was literally named "get() returns null for missing keys" -- it now covers the throw,
    and keeps a find() call to pin the null half of the contract.
  * 002 used getLocal() to show that local lookups do not reach the parent. That intent is
    unchanged; under the new contract it is expressed by the throw rather than by a null.
Implicit declaration slipped through a local -O0 build but breaks CI's -Werror.
@codecov

codecov Bot commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

… on a miss

The chaos harness detected an absent key by comparing get()/getLocal() against null, which
was the old behaviour. get()/getLocal() now throw ContextException on a miss, so absence is
detected by the throw instead:

  * "reads inherited context" — a locally-absent key is proven by getLocal() throwing, with
    findLocal() still pinning the null-returning half.
  * "replace and unset" — after unset(), get() must throw rather than answer null.

Regenerated fuzzy-tests/_generated/context passes 11/11; full _generated 724/724.
@EdmondDantes
EdmondDantes merged commit 1d10777 into main Jul 15, 2026
9 checks passed
@EdmondDantes
EdmondDantes deleted the context-get-must-throw branch July 15, 2026 09:02
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