WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years 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 years ago.
39054.2.diff (3.9 KB) - added by jblz 5 years ago.
Add beyond lower bound checks in int & number types

Download all attachments as: .zip

Change History (9)

@jblz
5 years ago

#1 @jnylen0
5 years 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 years ago

Add beyond lower bound checks in int & number types

#2 @jblz
5 years 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 years 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 years ago

#5 @dd32
5 years 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
5 years 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
5 years 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.