Repository navigation
Return INI values as written instead of substituting PHP constants - #248
Merged
Merged
Conversation
`Ini::parse()` hands its content to `parse_ini_string()` with the default scanner, which replaces any token matching the name of a defined PHP constant with that constant's value. The parser's output then depends on the state of the host process rather than on the document it was given: a settings block containing `title = PHP_VERSION` came back as the interpreter's version string instead of the text its author wrote, and a constant name used inside a bracketed key (`a[PHP_VERSION] = x`) became that value as an array key. This extends the placeholder technique the parser already uses for `${...}` interpolation - before parsing, a marker is prefixed to each token in the document that names a currently defined constant so the native parser cannot resolve it, and the marker is stripped again in a recursive restore walk (`restorePlaceholders()`, which runs after the existing `${` restoration). A token is only marked when `defined()` reports a constant of that name, and only as a whole token, so a document naming none of them is handed to the native parser byte for byte. Switching to `INI_SCANNER_RAW` was considered and rejected: it breaks multi-line quoted values, leaves `\"` escapes unprocessed and drops keyword coercion.
Behaviour changes:
- A token that happens to match the name of a defined PHP constant now stays as written, where it was previously replaced by that constant's value. This covers a token adjacent to a quoted run (`foo = CONST"x"`), repeated tokens in one value, tokens in array and nested-key values, and a constant name inside a bracketed key, which now becomes that key's own text. A bracketed key offset is also the one position where the native scanner resolves `on`, `off`, `yes`, `no` and `none` against the constant table rather than coercing them, so those are returned as written there too. The offset may be indented (`a[ yes] = x`): the native scanner skips that whitespace before it resolves the offset, so the marker does too.
- An INI operator expression over constant names - the `php.ini` bitmask idiom, `a = E_ALL & ~E_NOTICE` - is now evaluated as if those constants were undefined, so it yields `0` rather than a number computed from their values. Winter's own INI documents contain no such expression.
- A keyword-named constant standing between two `|` characters is now marked as well, because the offset rule applies to that delimiter too - `parsePreProcess()` uses it in keys. Where the previous parser lexed `a = b|yes|c` as a keyword and rejected the whole document, the value now parses as the bitwise-or of two unresolved tokens, `0`, which is the result `a = b|notakeyword|c` already gave. It takes a defined constant named `on`, `off`, `yes`, `no` or `none` to reach this at all.
- A failure inside the marking pre-pass, meaning a PCRE backtrack or recursion limit on a pathological document, now throws `RuntimeException`. Previously the pass could return null and that null would be handed to the native parser, which parses it as nothing at all.
- Everything else parses as before: in a value the keywords `true`, `false`, `on`, `off`, `yes`, `no`, `none` and `null` still coerce to `1`/`''`, and `true`, `false` and `null` do so in a bracketed key as well; quoted values, escape sequences and multi-line quoted values are unchanged; section headers and bare key names are untouched; `${...}` is still returned literally; and a document whose own text contains the parser's internal marker keeps it.
- `restoreDollarBrace()`, which has shipped since v1.2.13, keeps its name, its behaviour and its call contract: `parse()` still hands it the whole parsed array, so an override that walks array nodes as well as strings keeps working.
Tests in `tests/Parse/IniTest.php`: `testPhpConstantNamesAreNotSubstituted`, `testInterpreterConstantsAreNotSubstituted`, `testConstantNamesInBracketedKeysAreNotSubstituted`, `testKeywordNamedConstantsInBracketedKeysAreNotSubstituted`, `testKeywordsCoercedInBracketedKeysAreUnchanged`, `testQuotedConstantNamesAreUnchanged`, `testReservedKeywordsAreStillCoerced`, `testKeywordCoercionIsIndependentOfDefinedConstantNames`, `testShortConstantNamesAreNotMarkedInsideLongerTokens`, `testMultilineQuotedValuesAreUnaffected`, `testDocumentTextMatchingTheInternalMarkerIsPreserved`, `testReleasedRestoreHookIsStillCalled`, `testReleasedRestoreHookStillReceivesArrayNodes`, `testRenderedConstantNameRoundTrips`, `testKeywordNamedConstantsInIndentedBracketedKeysAreNotSubstituted` and `testAFailedIdentifierPrePassIsReported`.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe INI parser now escapes defined PHP constant names before calling Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 90.91% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 2 files. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely describes the main change: preserving INI values instead of substituting defined PHP constants. |
| Description Check | |
Check skipped - CodeRabbit’s high-level summary is enabled. |
✨ Finishing Touches 💡 1
-
🛠️ Fix failing CI checks 💡
-
- Commit to this branch
- Create a new PR
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
`testAFailedIdentifierPrePassIsReported` forced the failure by lowering `pcre.backtrack_limit`, which does not produce one on every PCRE build: it passed locally and failed on all eight CI jobs. The replacement is a seam — `replaceIdentifiers()` — that the test overrides to return null directly, so the guard is exercised the same way everywhere.
parsePreProcess() normalizes EOLs with a unicode-mode pattern whose return it did not check. Malformed UTF-8 fails that pattern deterministically, so the document was replaced with an empty string and parsed as nothing at all, without a diagnostic anywhere: the settings block of a CMS template or a mail template came back empty and the failure was invisible. It now throws, as the identifier pre-pass already did. SectionParser::parseSettings() catches that. Its `@` suppresses diagnostics but not exceptions, so the throw would otherwise escape a path deliberately written not to fail. An invalid settings section is still passed on as a special attribute, which the caller re-parses unsuppressed once it can name the file it came from, so the failure is reported with that object's own context rather than as a bare exception.
LukeTowers
added
the
maintenance
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.
Ini::parse()usesparse_ini_string()'s default scanner, which replaces any unquoted token naming a defined PHP constant with that constant's value —title = PHP_VERSIONcomes back as the interpreter's version rather than the text its author wrote. This extends the placeholder technique already used for${...}: before parsing, a marker is prefixed to each token thatdefined()reports, and stripped again afterwards.INI_SCANNER_RAWwas rejected — it breaks multi-line quoted values, leaves\"unprocessed and drops keyword coercion.It also fixes an unchecked
preg_replacereturn inparsePreProcess(), where malformed UTF-8 failed the unicode-mode EOL pattern and silently emptied the document. Both pre-passes now throw, andSectionParser::parseSettings()catches them, so an unparseable settings section still becomes theERROR_INIattribute instead of escaping its@ ... ?:.Behaviour changes
a = E_ALL & ~E_NOTICEnow yields0, those constants being treated as undefined. No Winter INI document uses the idiom.a = b|yes|cnow parses as0where it was previously rejected, and only if a constant namedyesis defined.RuntimeExceptioninstead of parsing nothing.${...}, andrestoreDollarBrace()'s name and call contract.Tests
19 new tests across
tests/Parse/IniTest.phpandtests/Halcyon/SectionParserTest.php, each proven to fail without its production change. Suite green at 858 tests / 9 skips (the twoHttpTestmarketplace-API tests need network access and fail identically ondevelop);phpcsclean. Parity-checked over 31 documents against the unpatched parser, differing only in the cases listed above. Cost on this checkout's 148 real settings blocks: 0.82 ms -> 1.99 ms.Summary by CodeRabbit