Better coercion #336

shmax · 2016-11-26T23:17:35Z

This is a second attempt at providing coercion support. My first attempt suffered from a few faults:

It piggy-backed on check()--which did not take a reference as a first argument--so only objects passed in as variables were affected by coercion.
Again due to the limitation on references not always being available, I had to sort of sneak coercion in here and there where I could, and it wasn't always possible for use cases like allOf

This new approach has a number of advantages and improvements over the older one:

added a new top-level coerce method to kick things off. It takes a reference, and everything after that point switches between coerce and check methods as needed. check is unaffected.
you can coerce non-objects... eg

$value = "false";
$schema = (object)[
  "type":"boolean"
];
$validator->coerce($value, $schema); // validates
echo get_type($value); // "boolean"

coercion now happens more deeply in the code, at the TypeConstraint level. This means that as long as the right coerce flags are passed along and references are used where appropriate internally, coercion just sort of works properly without having to wedge additional code in all over the place.
The CHECK_MODE_COERCE flag is no longer necessary, so you're no longer obliged to create a Factory, which means simpler client code in most cases.

Fixes #334, #329 and #322

API Note: while this does not break the check() API, you must now use coerce() to do coercion. The CHECK_MODE_COERCE flag has been removed.

Touched a lot of files, apologies, but hopefully it's not too hard to follow.

@bighappyface @pcjacobse @digitalkaoz please review

shmax · 2016-11-30T17:41:51Z

Please review!

digitalkaoz · 2016-12-01T06:04:52Z

I did... Max Loeb <[email protected]> schrieb am Mi., 30. Nov. 2016, 18:41:

…

Please review! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#336 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAR614rXX-NeGNX5oSlaySFCeXiARzCaks5rDbVhgaJpZM4K9BDN> .

shmax · 2016-12-01T06:54:27Z

@digitalkaoz Did you? I don't see any comments, stamps, or 'approved' events. Did I miss something?

digitalkaoz · 2016-11-28T08:25:35Z

src/JsonSchema/Constraints/CollectionConstraint.php

+	{
+		$this->_check($value, $schema, $path, $i, true);
+	}
+


you should fix the indentation here. maybe we should create a PR with php-cs-fixer & style-ci

digitalkaoz · 2016-11-28T08:27:25Z

src/JsonSchema/Constraints/Constraint.php

+			$validator->coerce($value, $schema, $path, $i);
+		} else {
+			$validator->check($value, $schema, $path, $i);
+		}


indentation again

digitalkaoz · 2016-11-28T08:31:54Z

src/JsonSchema/Constraints/CollectionConstraint.php

+		$this->_check($value, $schema, $path, $i, true);
+	}
+
+    protected function _check(&$value, $schema = null, JsonPointer $path = null, $i = null, $coerce = false)


mh maybe we can make it even more BC by just providing a CoerceConstraint (which wraps the underlying Constraint) and does all the type checking. so in my educated guess we only pass the coerce flag to the factory so it wraps the wanted constraint with the CoerceConstraint. The Facade-Pattern it is ;)

so all other constraints are mostly untouched...
like this:

class CoerceConstraint { public function __construct(Constraint $constraint) { $this->constraint = $constraint; } public function check(...) { $result = $this->constraint->check(...); //do the coercion here... } }

Interesting idea, but unfortunately any kind of coercion that depends on check is a non-starter because I would have to change the first argument to a reference, which would break bc.

digitalkaoz · 2016-12-01T09:14:00Z

ah now, forgot to submit the review

shmax · 2016-12-01T18:18:37Z

Fixed the whitespace issues, thanks; one of these days I should quit being lazy and modify my IDE settings.

shmax · 2016-12-07T04:59:41Z

@bighappyface How are your plans for the next release coming along?

bighappyface · 2016-12-07T15:32:00Z

4.1.0 with this PR?

shmax · 2016-12-07T16:37:38Z

Sounds good. Are we waiting on anything? I believe I'll need to rebase once you merge #331

bighappyface

@shmax please rebase

shmax · 2016-12-11T17:43:45Z

@bighappyface Rebased.

@SereneAnt I de-simplified your code a bit; I've brought in a TypeConstraint validator to replace the stuff you were doing inline because I need coercion to work. I don't think it should slow things down much, if at all, because it pretty much does the same things you were doing, only within its own domain (so important functionality like coercion happens). If it seems like a problem to you we can just add a check for $coerce to the conditions we consider before branching into your optimized code.

Please review!

SereneAnt · 2016-12-12T11:40:49Z

src/JsonSchema/Constraints/Constraint.php

@@ -125,10 +124,14 @@ protected function incrementPath(JsonPointer $path = null, $i)
     * @param JsonPointer|null $path
     * @param mixed            $i
     */
-    protected function checkArray($value, $schema = null, JsonPointer $path = null, $i = null)
+    protected function checkArray(&$value, $schema = null, JsonPointer $path = null, $i = null, $coerce = false)


Missing PHPDoc for parameter$coerse.
IMHO, $coerse flag would look better as the Factory field rather than additional method parameter.

Ah, good eye, thank you. I've added phpdocs, following the patterns and style already in place.

As for adding $coerce as a Factory flag, well, that was the approach I tried previously. It made sense in my previous approach because I was driving everything through check, and it just became a mode flag (stored in Factory, as you suggest). The problem there was that I had no reference to work with in check, so I couldn't properly coerce in all situations.

Now that I'm driving it through an explicit function call, coerce, it's no longer really a configuration value. If one were to, say, call check immediately after calling coerce, I would just have to set any stored fields back to false, so it would be another moving part with no real value.

I was actually interested in your reaction to what I did in CollectionConstraint. Any thoughts?

Changes look reasonable. Type check is more solid with this approach.
The performance penalty is not too big, avg time for validating 100K string array:

original implementation: 1700 ms

optimized, explicit type check (string, number, int) improve validation performance of long string, number and integer arrays #331: 550 ms

this branch: 720 ms

Thanks for the review! If you're satisfied, please approve.

I have no write permissions, can't approve.

A LGTM will help

shmax · 2016-12-14T05:06:15Z

@bighappyface please merge, or let me know what I have to do to move forward.

SereneAnt · 2016-12-14T18:20:40Z

LGTM

pcjacobse · 2016-12-21T15:27:45Z

Sorry for the late reply, but works like a charm ;)

shmax · 2016-12-22T06:30:41Z

@bighappyface What's the plan?

shmax · 2016-12-29T00:56:20Z

Thank you!

alcohol · 2017-02-16T11:01:36Z

Nobody noticed the mixed spaces and tabs indentations?

shmax · 2017-02-16T16:58:52Z

Nope, nobody did, and you're commenting on a PR that was merged almost two months ago. If you can find formatting issues in the current state of the code I'll be happy to clean 'em up.

alcohol · 2017-02-17T09:21:19Z

Yeah, sorry, I was going through the changes between 4.x and 5.x as I wanted to see if Composer can easily upgrade/be compatible with the latest major version (we currently are compatible with 3.x and 4.x). Noticed some inconsistencies in the change sets. Hence my code-style PR submission :-)

bighappyface · 2017-02-17T14:16:31Z

Thanks for the help ensuring things stay smooth with Composer, @alcohol.

Being a dependency for Composer is our crown jewel 👑

digitalkaoz suggested changes Dec 1, 2016

View reviewed changes

bighappyface approved these changes Dec 8, 2016

View reviewed changes

Better coercion

1224341

SereneAnt reviewed Dec 12, 2016

View reviewed changes

add phpdocs

8885faf

bighappyface merged commit d39c56a into jsonrainbow:master Dec 22, 2016

shmax deleted the coerce-v2 branch December 22, 2016 16:53

shmax mentioned this pull request Jan 13, 2017

Allow coercion to boolean from '0' and '1' #345

Closed

Better coercion #336

Better coercion #336

Uh oh!

Conversation

shmax commented Nov 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shmax commented Nov 30, 2016

Uh oh!

digitalkaoz commented Dec 1, 2016 via email

Uh oh!

shmax commented Dec 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

digitalkaoz commented Dec 1, 2016

Uh oh!

shmax commented Dec 1, 2016

Uh oh!

shmax commented Dec 7, 2016

Uh oh!

bighappyface commented Dec 7, 2016

Uh oh!

shmax commented Dec 7, 2016

Uh oh!

bighappyface left a comment

Choose a reason for hiding this comment

Uh oh!

shmax commented Dec 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmax Dec 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmax commented Dec 14, 2016

Uh oh!

SereneAnt commented Dec 14, 2016

Uh oh!

pcjacobse commented Dec 21, 2016

Uh oh!

shmax commented Dec 22, 2016

Uh oh!

shmax commented Dec 29, 2016

Uh oh!

alcohol commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shmax commented Feb 16, 2017

Uh oh!

alcohol commented Feb 17, 2017

Uh oh!

bighappyface commented Feb 17, 2017

Uh oh!

Uh oh!

shmax commented Nov 26, 2016 •

edited

Loading

shmax commented Dec 11, 2016 •

edited

Loading

shmax Dec 12, 2016 •

edited

Loading

alcohol commented Feb 16, 2017 •

edited

Loading