Skip to content

Add constraint factory #143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 8, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 28 additions & 9 deletions src/JsonSchema/Constraints/Constraint.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,21 @@ abstract class Constraint implements ConstraintInterface
const CHECK_MODE_NORMAL = 1;
const CHECK_MODE_TYPE_CAST = 2;

/**
* @var null|Factory
*/
private $factory;

/**
* @param int $checkMode
* @param UriRetriever $uriRetriever
* @param Factory $factory
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing |null

*/
public function __construct($checkMode = self::CHECK_MODE_NORMAL, UriRetriever $uriRetriever = null)
public function __construct($checkMode = self::CHECK_MODE_NORMAL, UriRetriever $uriRetriever = null, Factory $factory = null)
{
$this->checkMode = $checkMode;
$this->uriRetriever = $uriRetriever;
$this->factory = $factory;
}

/**
Expand All @@ -50,6 +57,18 @@ public function getUriRetriever()
return $this->uriRetriever;
}

/**
* @return Factory
*/
public function getFactory()
{
if (!$this->factory) {
$this->factory = new Factory($this->getUriRetriever());
}

return $this->factory;
}

/**
* @param UriRetriever $uriRetriever
*/
Expand Down Expand Up @@ -137,7 +156,7 @@ protected function incrementPath($path, $i)
*/
protected function checkArray($value, $schema = null, $path = null, $i = null)
{
$validator = new CollectionConstraint($this->checkMode, $this->uriRetriever);
$validator = $this->getFactory()->createInstanceFor('collection');
$validator->check($value, $schema, $path, $i);

$this->addErrors($validator->getErrors());
Expand All @@ -154,7 +173,7 @@ protected function checkArray($value, $schema = null, $path = null, $i = null)
*/
protected function checkObject($value, $schema = null, $path = null, $i = null, $patternProperties = null)
{
$validator = new ObjectConstraint($this->checkMode, $this->uriRetriever);
$validator = $this->getFactory()->createInstanceFor('object');
$validator->check($value, $schema, $path, $i, $patternProperties);

$this->addErrors($validator->getErrors());
Expand All @@ -170,7 +189,7 @@ protected function checkObject($value, $schema = null, $path = null, $i = null,
*/
protected function checkType($value, $schema = null, $path = null, $i = null)
{
$validator = new TypeConstraint($this->checkMode, $this->uriRetriever);
$validator = $this->getFactory()->createInstanceFor('type');
$validator->check($value, $schema, $path, $i);

$this->addErrors($validator->getErrors());
Expand All @@ -186,7 +205,7 @@ protected function checkType($value, $schema = null, $path = null, $i = null)
*/
protected function checkUndefined($value, $schema = null, $path = null, $i = null)
{
$validator = new UndefinedConstraint($this->checkMode, $this->uriRetriever);
$validator = $this->getFactory()->createInstanceFor('undefined');
$validator->check($value, $schema, $path, $i);

$this->addErrors($validator->getErrors());
Expand All @@ -202,7 +221,7 @@ protected function checkUndefined($value, $schema = null, $path = null, $i = nul
*/
protected function checkString($value, $schema = null, $path = null, $i = null)
{
$validator = new StringConstraint($this->checkMode, $this->uriRetriever);
$validator = $this->getFactory()->createInstanceFor('string');
$validator->check($value, $schema, $path, $i);

$this->addErrors($validator->getErrors());
Expand All @@ -218,7 +237,7 @@ protected function checkString($value, $schema = null, $path = null, $i = null)
*/
protected function checkNumber($value, $schema = null, $path = null, $i = null)
{
$validator = new NumberConstraint($this->checkMode, $this->uriRetriever);
$validator = $this->getFactory()->createInstanceFor('number');
$validator->check($value, $schema, $path, $i);

$this->addErrors($validator->getErrors());
Expand All @@ -234,15 +253,15 @@ protected function checkNumber($value, $schema = null, $path = null, $i = null)
*/
protected function checkEnum($value, $schema = null, $path = null, $i = null)
{
$validator = new EnumConstraint($this->checkMode, $this->uriRetriever);
$validator = $this->getFactory()->createInstanceFor('enum');
$validator->check($value, $schema, $path, $i);

$this->addErrors($validator->getErrors());
}

protected function checkFormat($value, $schema = null, $path = null, $i = null)
{
$validator = new FormatConstraint($this->checkMode, $this->uriRetriever);
$validator = $this->getFactory()->createInstanceFor('format');
$validator->check($value, $schema, $path, $i);

$this->addErrors($validator->getErrors());
Copy link
Contributor

Choose a reason for hiding this comment

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

As a thought - some of the duplication of this class could now be moved into a private method. checkObject would need to remain as it is - as it also passes $patternProperties

    protected function checkFormat($value, $schema = null, $path = null, $i = null)
    {
        $this->checkByType('format', $value, $schema, $path, $i);
    }

    /**
     * Create a validator of type $type and validate value against it
     * 
     * @param string $type
     * @param mixed  $value
     * @param mixed  $schema
     * @param mixed  $path
     * @param mixed  $i
     */
    private function checkByType($type, $value, $schema, $path, $i)
    {
        $validator = $this->getFactory()->factory($type);
        $validator->check($value, $schema, $path, $i);

        $this->addErrors($validator->getErrors());
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be something for a different PR

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed.

Expand Down
81 changes: 81 additions & 0 deletions src/JsonSchema/Constraints/Factory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
<?php

/*
* This file is part of the JsonSchema package.
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace JsonSchema\Constraints;

use JsonSchema\Exception\InvalidArgumentException;
use JsonSchema\Uri\UriRetriever;
use JsonSchema\Validator;

/**
* Factory for centralize constraint initialization.
*/
class Factory
Copy link
Contributor

Choose a reason for hiding this comment

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

This library appears to duplicate the namespace in the class name.

For example:
JsonSchema\Uri\UriRetriever instead of JsonSchema\Uri\Retriever
JsonSchema\Constraints\NumberConstraint instead of JsonSchema\Constraints\Number

This should maybe be called ConstraintFactory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to be just a cosmetic thing. Prefer to hear other opinions

Copy link
Contributor

Choose a reason for hiding this comment

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

@bighappyface - one for you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am okay with the name, but I think we would be best served to stick with the convention established in release 1.4.0: https://github.com/justinrainbow/json-schema/releases/tag/1.4.0 (PR #136)

That being said, I think the suggestion @alecsammon put forth is good guidance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well there are lot of opinions and this could be a large discussion.

  1. [Proposal] new constraints name - for PHP7 #136 It's a workaround for a technical issue.
  2. Recently a PHP 7 RFC has been approved relaxing reserved words based in context.
  3. The purpose of namespaces is IMO to have clean class names.

{
/**
* @var UriRetriever
*/
protected $uriRetriever;

/**
* @param UriRetriever $uriRetriever
*/
public function __construct(UriRetriever $uriRetriever = null)
{
if (!$uriRetriever) {
$uriRetriever = new UriRetriever();
}

$this->uriRetriever = $uriRetriever;
}

/**
* @return UriRetriever
*/
public function getUriRetriever()
{
return $this->uriRetriever;
}

/**
* Create a constraint instance for the given constraint name.
*
* @param string $constraintName
* @return ConstraintInterface|ObjectConstraint
* @throws InvalidArgumentException if is not possible create the constraint instance.
*/
public function createInstanceFor($constraintName)
{
switch ($constraintName) {
case 'array':
case 'collection':
return new CollectionConstraint(Constraint::CHECK_MODE_NORMAL, $this->uriRetriever, $this);
case 'object':
return new ObjectConstraint(Constraint::CHECK_MODE_NORMAL, $this->uriRetriever, $this);
case 'type':
return new TypeConstraint(Constraint::CHECK_MODE_NORMAL, $this->uriRetriever, $this);
case 'undefined':
return new UndefinedConstraint(Constraint::CHECK_MODE_NORMAL, $this->uriRetriever, $this);
case 'string':
return new StringConstraint(Constraint::CHECK_MODE_NORMAL, $this->uriRetriever, $this);
case 'number':
return new NumberConstraint(Constraint::CHECK_MODE_NORMAL, $this->uriRetriever, $this);
case 'enum':
return new EnumConstraint(Constraint::CHECK_MODE_NORMAL, $this->uriRetriever, $this);
case 'format':
return new FormatConstraint(Constraint::CHECK_MODE_NORMAL, $this->uriRetriever, $this);
case 'schema':
return new SchemaConstraint(Constraint::CHECK_MODE_NORMAL, $this->uriRetriever, $this);
case 'validator':
return new Validator(Constraint::CHECK_MODE_NORMAL, $this->uriRetriever, $this);
}

throw new InvalidArgumentException('Unknown constraint ' . $constraintName);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line appears to be untested - here is a test :)

    /**
     * @expectedException JsonSchema\Exception\InvalidArgumentException
     */
    public function testInvalidConstraint()
    {
        $this->factory->factory('badConstraintName');
    }

}
}
2 changes: 1 addition & 1 deletion src/JsonSchema/Validator.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class Validator extends Constraint
*/
public function check($value, $schema = null, $path = null, $i = null)
{
$validator = new SchemaConstraint($this->checkMode, $this->uriRetriever);
$validator = $this->getFactory()->createInstanceFor('schema');
$validator->check($value, $schema);

$this->addErrors(array_unique($validator->getErrors(), SORT_REGULAR));
Expand Down
78 changes: 78 additions & 0 deletions tests/JsonSchema/Tests/Constraints/FactoryTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<?php

/*
* This file is part of the JsonSchema package.
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace JsonSchema\Tests\Constraints;

use JsonSchema\Constraints\Factory;
use PHPUnit_Framework_TestCase as TestCase;

class FactoryTest extends TestCase
{
/**
* @var Factory
*/
protected $factory;

protected function setUp()
{
$this->factory = new Factory();
}

/**
* @dataProvider constraintNameProvider
*
* @param string $constraintName
* @param string $expectedClass
* @return void
*/
public function testCreateInstanceForConstraintName($constraintName, $expectedClass)
{
$constraint = $this->factory->createInstanceFor($constraintName);

$this->assertInstanceOf($expectedClass, $constraint);
$this->assertInstanceOf('JsonSchema\Constraints\ConstraintInterface', $constraint);
$this->assertSame($this->factory->getUriRetriever(), $constraint->getUriRetriever());
}

public function constraintNameProvider()
{
return array(
array('array', 'JsonSchema\Constraints\CollectionConstraint'),
array('collection', 'JsonSchema\Constraints\CollectionConstraint'),
array('object', 'JsonSchema\Constraints\ObjectConstraint'),
array('type', 'JsonSchema\Constraints\TypeConstraint'),
array('undefined', 'JsonSchema\Constraints\UndefinedConstraint'),
array('string', 'JsonSchema\Constraints\StringConstraint'),
array('number', 'JsonSchema\Constraints\NumberConstraint'),
array('enum', 'JsonSchema\Constraints\EnumConstraint'),
array('format', 'JsonSchema\Constraints\FormatConstraint'),
array('schema', 'JsonSchema\Constraints\SchemaConstraint'),
array('validator', 'JsonSchema\Validator'),
);
}

/**
* @dataProvider invalidConstraintNameProvider
*
* @param string $constraintName
* @return void
*/
public function testExceptionWhenCreateInstanceForInvalidConstraintName($constraintName)
{
$this->setExpectedException('JsonSchema\Exception\InvalidArgumentException');
$this->factory->createInstanceFor($constraintName);
}

public function invalidConstraintNameProvider()
{
return array(
array('invalidConstraintName'),
);
}
}