fix: Report a type mismatch for non-numeric flag values - #29
Conversation
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
@cursor review |
| } | ||
|
|
||
| return $this->detailsConverter->toResolutionDetails($result); | ||
| if ($flagValueType == FlagValueType::INTEGER) { |
There was a problem hiding this comment.
It's been a bit since I've looked at this part of the algorithm, but do we typically do this truncation?
There was a problem hiding this comment.
Yes — truncation is what the other server providers do, which is why I matched it rather than treating a non-integral float as a type mismatch:
- Python
_LaunchDarklyProvider__validate_and_cast_value:return int(value) # Float decimals are truncated to intforFlagType.INTEGER. - Ruby
provider.rbinteger resolution:evaluation_detail.value.to_i. - The JS providers have only
resolveNumberEvaluation, so the question does not arise there.
Both accept an int or a float and reject bools and numeric strings, which is exactly the shape of the check here.
Worth being explicit that truncation is not what this PR is really about, though: before it, is_numeric let a non-integral float through uncast, so resolveIntegerValue returned 5.5 and the OpenFeature client raised a TypeError that surfaced as GENERAL rather than anything meaningful. So the choice is between truncating like Python and Ruby, or returning TYPE_MISMATCH with the default value. I went with the former for cross-SDK consistency, but if you would rather a 5.5 integer flag be a hard TYPE_MISMATCH here I am happy to switch it — it is a one-line change plus the test, and it would be a deliberate divergence from Python and Ruby.
Adds a feature matrix to the README, mirroring the OpenFeature PHP SDK's own matrix, so a reader can see at a glance what this provider does and does not do. - Most unsupported rows are limitations of the OpenFeature PHP SDK, not of this provider — eventing, shutdown, named clients, tracking, transaction context and flag metadata have no place in that SDK's provider interface — and each row says so, so they are not read as provider gaps. - Format matches the matrices added to the Java, Python, .NET and JavaScript providers this week, so the providers can be compared side by side. <details> <summary>Implementation details</summary> Rows and statuses were checked against the OpenFeature PHP SDK 2.3.0 and the LaunchDarkly PHP SDK 6.8.3 rather than copied from another provider: `OpenFeature\interfaces\provider\Provider` declares only the five `resolve*Value` methods plus hooks, logger and metadata accessors, `ProviderAware` holds a single provider for the whole API, and neither `EvaluationDetails` nor `ResolutionDetails` carries flag metadata. The three separate fixes opened alongside this (#27, #28, #29) are provider-side bugs within rows the matrix marks as supported, so they do not change any status here. **Requirements** - [x] I have added test coverage for new or changed functionality - [x] I have followed the repository's [pull request submission guidelines](../blob/main/CONTRIBUTING.md#submitting-pull-requests) - [x] I have validated my changes against all supported platform versions **Related issues** None. **Describe the solution you've provided** A `## Feature matrix` section after the supported-versions section, with the same `Status | Feature | Notes` shape as the other LaunchDarkly OpenFeature providers. **Describe alternatives you've considered** Listing only the rows the OpenFeature PHP SDK's own matrix has: it would drop initialization, tracking, transaction context and flag metadata, which are exactly the rows readers coming from another LaunchDarkly provider will look for. **Additional context** Testing: documentation only; `make check` still passes. </details> Link to Devin session: https://app.devin.ai/sessions/38a6eaf69fcf41109e136a1d0fe5e899 Open in Devin Desktop: https://app.devin.ai/desktop/session/38a6eaf69fcf41109e136a1d0fe5e899?variant=devin Requested by: @kinyoklion <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Adds a **Feature matrix** section to the README (after supported PHP versions), aligned with the [OpenFeature PHP SDK feature list](https://github.com/open-feature/php-sdk#-features) and the same table shape used on other LaunchDarkly OpenFeature providers. > > The table lists each capability with ✅/❌ status and notes: supported areas (flag types, targeting, hooks, logging, MultiProvider, extending via `LDClient`) and unsupported rows that call out whether the gap is the OpenFeature PHP SDK (e.g. named clients, eventing, shutdown, tracking, transaction context, flag metadata) versus provider behavior. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit da8803a. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Numeric flag resolution now accepts only numeric flag values, and converts them to the requested type.
is_numericaccepted numeric strings, so a flag whose value is"5"was returned as a string fromresolveIntegerValue. The OpenFeature client'sgetIntegerValue(): intthen threw aTypeErrorinternally, which it swallows per spec requirement 1.4.9 and reports as error codeGENERAL— instead of theTYPE_MISMATCHthe provider should have reported.int(a float is truncated) and a float flag to afloat, rather than passing the raw JSON type through.Implementation details
Because the value is converted, the result handed to the details converter is a new
EvaluationDetailcarrying the converted value with the original variation index and reason.EvaluationDetail::isDefaultValue()is derived from the variation index, which is preserved, so the variant of the resolution details is unchanged.Found during the weekly OpenFeature provider audit.
Requirements
Related issues
None.
Describe the solution you've provided
The integer and float branches check
is_int/is_floatinstead ofis_numeric, and the accepted value is cast to the requested type. Rejected values continue through the existingmismatchedTypeDetailspath, so they reportTYPE_MISMATCHwith the default value.Describe alternatives you've considered
Keeping
is_numericand casting numeric strings as well: that hides a genuinely mistyped flag instead of reporting it, and disagrees with the other providers.Additional context
Testing:
make check(composer cs-check,composer phpstan,composer phpunit). The existing type-matching data set gains numeric-string and non-integral-float cases and now asserts withassertSame, so the resolved type is checked and not just its loose value.Link to Devin session: https://app.devin.ai/sessions/38a6eaf69fcf41109e136a1d0fe5e899
Open in Devin Desktop: https://app.devin.ai/desktop/session/38a6eaf69fcf41109e136a1d0fe5e899?variant=devin
Requested by: @kinyoklion
Note
Overview
Integer and float flag resolution no longer treats numeric strings as valid: type checks use
is_int/is_floatinstead ofis_numeric, so a flag value like"5"now returns the default withTYPE_MISMATCHinstead of a string that could surface as aGENERALerror inside the OpenFeature client.Accepted numeric values are cast to the requested type (
inttruncates floats;floatpromotes ints), and the provider passes a newEvaluationDetailwith the converted value while preserving variation index and reason.Tests add numeric-string and float-to-int cases, switch type-matching assertions to
assertSame, and covervalueForAll("5")for integer/float resolution errors.Reviewed by Cursor Bugbot for commit 0685543. Bugbot is set up for automated code reviews on this repo. Configure here.