Skip to content

fix: Number component incorrectly uses label as validator key#495

Merged
nie7321 merged 1 commit into
NIT-Administrative-Systems:developfrom
77media-creations:fix/number-component-validation
Apr 16, 2026
Merged

fix: Number component incorrectly uses label as validator key#495
nie7321 merged 1 commit into
NIT-Administrative-Systems:developfrom
77media-creations:fix/number-component-validation

Conversation

@77media-creations

Copy link
Copy Markdown
Contributor

Problem

The Number component's processValidations method overwrites the $fieldKey
parameter with the component's label string:

$fieldKey = $this->label() ?? $this->key();

This means the validator's data array is keyed by the label (e.g. "Wash Temp")
instead of the actual FormIO field key (e.g. washTemp).

When a user submits an empty optional number field, submissionValue() returns
null. Laravel's Arr::has finds the label-key directly in the data (since it
contains no dots), so the numeric rule fires on null, producing a spurious
validation error: "The Wash Temp must be a number."

Odd side effect: Labels ending with a dot (e.g. "Rinse Temp.") happen to
avoid the error — Laravel's Arr::has treats dots as nested key separators, so it
looks for data["Rinse Temp"][""] which doesn't exist, and the numeric rule
never fires. This made the bug appear inconsistent and hard to trace.

Fix

Two changes, modelled after the existing Currency component which handles this
correctly:

  1. Remove the $fieldKey override — use the parameter as passed in from
    BaseComponent::validate(), which is already $this->key().
  2. Add nullable for non-required fields — so that a null submission value
    does not trigger the numeric rule (same pattern as Currency).

Tests

Added two cases to NumberTest::validationsProvider() covering empty optional
fields (both null and empty string).

@77media-creations 77media-creations requested a review from a team as a code owner April 16, 2026 02:00

@dxnter dxnter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I think CI is misconfigured for forks, but I've verified that tests are passing

@nie7321 nie7321 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@nie7321 nie7321 merged commit 45e3a8a into NIT-Administrative-Systems:develop Apr 16, 2026
1 of 4 checks passed
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.

3 participants