Skip to content

Conversation

localheinz
Copy link
Contributor

This PR

  • marks Validator::check() and Validator::coerce() as deprecated

try {
$validator = new JsonSchema\Validator();
$validator->check($data, $schema);
$validator->validate($data, $schema);
Copy link
Contributor Author

@localheinz localheinz Dec 30, 2017

Choose a reason for hiding this comment

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

Probably makes sense to use Validator::validate() here.

// Validate
$validator = new JsonSchema\Validator();
$validator->check($data, (object) array('$ref' => 'file://' . realpath('schema.json')));
$validator->validate($data, (object) array('$ref' => 'file://' . realpath('schema.json')));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably makes sense to use Validator::validate() here, too.

@localheinz localheinz force-pushed the fix/deprecate branch 2 times, most recently from d54234b to 3b34779 Compare December 30, 2017 18:50
$v = new Validator();
$this->setExpectedException('\JsonSchema\Exception\InvalidArgumentException');
$v->check(json_decode('{}'), json_decode(''));
$v->validate(json_decode('{}'), json_decode(''));
Copy link
Contributor

Choose a reason for hiding this comment

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

You cannot use validate() in this situation, as the first argument is a reference. If you want to change this, you'll need to decode the document into an intermediate variable first, and then pass the variable.

}

public function testCheck()
public function testDeprecatedCheckStillValidates()
Copy link
Contributor

Choose a reason for hiding this comment

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

Method name needs clarification - validation in this test is supposed to fail, but the new name falsely implies that success is the intended outcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about I rename both to test.*DelegatesToValidate()?

Copy link
Contributor

Choose a reason for hiding this comment

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

That works :-). I'm not particularly fussed what they are named, as long as they're not ambiguous and give a reasonable idea of the intended purpose.

@localheinz
Copy link
Contributor Author

@erayd

Updated!

@bighappyface bighappyface merged commit 7d7f43a into jsonrainbow:master Dec 31, 2017
@localheinz localheinz deleted the fix/deprecate branch January 1, 2018 00:55
@localheinz
Copy link
Contributor Author

Thank you, @bighappyface and @erayd!

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