feat(context): get()/getLocal() throw on a missing key instead of returning null#195
Merged
Conversation
…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 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
The problem
get()was an exact duplicate offind(), andgetLocal()offindLocal(). Four methods, two behaviours:Both walked the parent Scopes; both answered
nullon a miss. Half of the API had no reason to exist.And three sources described three different things:
find(), this method throws if the key is not found at any level. Useget()when the value is mandatory." — and@throws Async\ContextExceptionfind(). Returnsnull.ContextExceptiondoes not exist.The site was describing the useful contract — the standard
find/getsplit (cf. PSR-11) — and the code simply never implemented it.The change
find()/findLocal()— the optional-value forms. Unchanged: they answernull.get()/getLocal()— the mandatory-value forms. A missing key now raisesAsync\ContextException, the class the docs already referred to.Parent lookup is untouched —
get()still reaches inherited Scopes, exactly likefind(). Only the miss differs.Registration order
async_register_context_ce()ran beforeasync_register_exceptions_ce(), soContextExceptionwas handed aNULLparent and trippedExceptions 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.phptwas 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 afind()call so the null half of the contract stays pinned.002-context_inheritance.phptusedgetLocal()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 anull.findLocal()still pins the null.Tests
011—get()/getLocal()throw,find()/findLocal()answernull, present keys still return,ContextException extends AsyncException.012—get()reaches parent Scopes;getLocal()does not; object keys are reported by class name.context12/12. Fullext/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
ContextExceptionin the docs were left alone on purpose while this was pending; they are now correct.