WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 4 months ago

Last modified 4 months ago

#39054 closed defect (bug) (fixed)

REST API: Improve error messages for number relational validation

Reported by: jblz Owned by: SergeyBiryukov
Milestone: 4.8 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests commit
Focuses: rest-api Cc:

Description

For a simple relation (i.e. when not describing that a value should be between two bounds), using the exclusive & inclusive modifiers aren't as readable as they could be.

When the relation is exclusive, greater than implies the exclusivity.
When the relation is inclusive, greater than or equal to would be easier to read.

For example, calling /media?page=0 currently returns: page must be greater than 1 (inclusive).
I'd expect it to say: page must be greater than or equal to 1

Attachments (2)

39054.diff (2.8 KB) - added by jblz 5 months ago.
39054.2.diff (3.9 KB) - added by jblz 5 months ago.
Add beyond lower bound checks in int & number types

Download all attachments as: .zip

Change History (9)

@jblz
5 months ago

#1 @jnylen0
5 months ago

  • Keywords has-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.7.1
  • Type changed from enhancement to defect (bug)

Patch looks good to me, thanks @jblz! Setting to "bug" because this language is mathematically incorrect.

It would be good to get some more tests for this behavior, it's a little concerning that only one random test case was affected. Any extra tests could probably go in these files:

tests/phpunit/tests/rest-api/rest-schema-sanitization.php
tests/phpunit/tests/rest-api/rest-schema-validation.php

@jblz
5 months ago

Add beyond lower bound checks in int & number types

#2 @jblz
5 months ago

Thanks for taking a look & for the pointer to the schema tests, @jnylen0.

There's already some coverage around the general behavior in `test_type_number` & `test_type_integer`. I updated the patch to include a check when the value is below the minimum bound.

Do you think we should update the above tests to check the message similar to the test I had to update here already or do you think the updated patch suffices?

#3 @jnylen0
5 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

There's already some coverage around the general behavior

In my previous comment I forgot that we don't generally test the text of error messages, just the error code (and any extra data associated with parameters). So I'm not too worried about it either way. 39054.2.diff LGTM for 4.7.1.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


5 months ago

#5 @dd32
5 months ago

  • Keywords commit added
  • Milestone changed from 4.7.1 to 4.8

As this only concerns strings, I'm pushing this into 4.8. We don't normally add (/change) strings in point releases, and this doesn't seem urgent enough to warrant changing that expectation.

39054.2.diff Looks good to me though.

#6 @SergeyBiryukov
4 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 39896:

REST API: Improve error messages for number relational validation.

Props jblz.
Fixes #39054.

#7 @SergeyBiryukov
4 months ago

In 39899:

REST API: Update unit tests to account for the string changes in [39896].

See #39054.

Note: See TracTickets for help on using tickets.