Fix explicit null value handling in compat mode - #2151
Merged
kibertoad merged 1 commit intoJun 14, 2026
Conversation
theodorejb
force-pushed
the
fix-compat-explicit-null
branch
from
April 30, 2026 02:32
b54ff13 to
51f0151
Compare
theodorejb
added a commit
to devtheorem/php-handlebars
that referenced
this pull request
Apr 30, 2026
I also opened handlebars-lang/handlebars.js#2151 to apply the same fix to Handlebars.js.
Follow-up to handlebars-langGH-2150. `container.lookup` used `result != null` which caused explicit null values to fall through to parent contexts. This was inconsistent with expected behavior and with Mustache, which stops and returns a key when present in the object, regardless of its value. The whole point of `compat` mode is to match Mustache's lookup semantics, so I don't think this behavior was intended. There were no tests for `compat` with an explicit null value, apart from a couple `compat` + `strict` mode tests I just added myself in handlebars-langGH-2150. One of these tests ("should still perform recursive lookup with a multi-part path not in context") was inconsistent with the test immediately below it ("should directly return an explicitly null property"), and that is fixed here. This change makes it possible for unify the lookup logic in `container.lookup` and `container.strictLookup`, additionally fixing an issue where `strictLookup` didn't respect the `allowedProtoMethods` runtime option the same as `lookup` (see the added test in `spec/security.js` compared to the test above it). This also fixes Map value resolution in compat-mode depthed lookup (included test by @TorMatzAndren from handlebars-lang#2156).
theodorejb
force-pushed
the
fix-compat-explicit-null
branch
from
May 17, 2026 17:30
51f0151 to
acaa839
Compare
Contributor
Author
|
@jaylinski @kibertoad By any chance can this be reviewed? |
kibertoad
approved these changes
Jun 14, 2026
Contributor
|
thank you! |
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
2 participants
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.
Follow-up to GH-2150.
container.lookupusedresult != nullwhich caused explicit null values to fall through to parent contexts. This was inconsistent with expected behavior and with Mustache, which stops and returns a key when present in the object, regardless of its value. The whole point ofcompatmode is to match Mustache's lookup semantics, so I don't think this behavior was intended.There were no tests for
compatwith an explicit null value, apart from a couplecompat+strictmode tests I just added myself in GH-2150. One of these tests ("should still perform recursive lookup with a multi-part path not in context") was inconsistent with the test immediately below it ("should directly return an explicitly null property"), and that is fixed here.This change makes it possible for unify the lookup logic in
container.lookupandcontainer.strictLookup, additionally fixing an issue wherestrictLookupdidn't respect theallowedProtoMethodsruntime option the same aslookup(see the added test inspec/security.jscompared to the test above it).Also fixed compat mode
Mapvalue resolution, and included the test for it from #2156 (if that is merged first, I can rebase this to fix the merge conflict).