-
Notifications
You must be signed in to change notification settings - Fork 357
New way of handling references #277
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,13 +10,20 @@ | |
namespace JsonSchema\Constraints; | ||
|
||
use JsonSchema\Exception\InvalidArgumentException; | ||
use JsonSchema\SchemaStorage; | ||
use JsonSchema\Uri\UriRetriever; | ||
use JsonSchema\UriRetrieverInterface; | ||
|
||
/** | ||
* Factory for centralize constraint initialization. | ||
*/ | ||
class Factory | ||
{ | ||
/** | ||
* @var SchemaStorage | ||
*/ | ||
protected $schemaStorage; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We've a getter method for $uriRetriever, wouldn't it make sense to have one for |
||
|
||
/** | ||
* @var UriRetriever $uriRetriever | ||
*/ | ||
|
@@ -50,34 +57,39 @@ class Factory | |
); | ||
|
||
/** | ||
* @param UriRetriever $uriRetriever | ||
* @param SchemaStorage $schemaStorage | ||
* @param UriRetrieverInterface $uriRetriever | ||
* @param int $checkMode | ||
*/ | ||
public function __construct(UriRetriever $uriRetriever = null, $checkMode = Constraint::CHECK_MODE_NORMAL) | ||
{ | ||
if (!$uriRetriever) { | ||
$uriRetriever = new UriRetriever(); | ||
} | ||
|
||
$this->uriRetriever = $uriRetriever; | ||
public function __construct( | ||
SchemaStorage $schemaStorage = null, | ||
UriRetrieverInterface $uriRetriever = null, | ||
$checkMode = Constraint::CHECK_MODE_NORMAL | ||
) { | ||
$this->uriRetriever = $uriRetriever ?: new UriRetriever; | ||
$this->schemaStorage = $schemaStorage ?: new SchemaStorage($this->uriRetriever); | ||
$this->checkMode = $checkMode; | ||
} | ||
|
||
/** | ||
* @return UriRetriever | ||
* @return UriRetrieverInterface | ||
*/ | ||
public function getUriRetriever() | ||
{ | ||
return $this->uriRetriever; | ||
} | ||
|
||
public function getSchemaStorage() | ||
{ | ||
return $this->schemaStorage; | ||
} | ||
|
||
public function getTypeCheck() | ||
{ | ||
if (!isset($this->typeCheck[$this->checkMode])) { | ||
if ($this->checkMode === Constraint::CHECK_MODE_TYPE_CAST) { | ||
$this->typeCheck[Constraint::CHECK_MODE_TYPE_CAST] = new TypeCheck\LooseTypeCheck(); | ||
} else { | ||
$this->typeCheck[$this->checkMode] = new TypeCheck\StrictTypeCheck(); | ||
} | ||
$this->typeCheck[$this->checkMode] = $this->checkMode === Constraint::CHECK_MODE_TYPE_CAST | ||
? new TypeCheck\LooseTypeCheck | ||
: new TypeCheck\StrictTypeCheck; | ||
} | ||
|
||
return $this->typeCheck[$this->checkMode]; | ||
|
@@ -112,7 +124,12 @@ public function setConstraintClass($name, $class) | |
public function createInstanceFor($constraintName) | ||
{ | ||
if (array_key_exists($constraintName, $this->constraintMap)) { | ||
return new $this->constraintMap[$constraintName]($this->checkMode, $this->uriRetriever, $this); | ||
return new $this->constraintMap[$constraintName]( | ||
$this->checkMode, | ||
$this->schemaStorage, | ||
$this->uriRetriever, | ||
$this | ||
); | ||
} | ||
throw new InvalidArgumentException('Unknown constraint ' . $constraintName); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,7 +127,7 @@ public function validateElement($element, $matches, $objectDefinition = null, Js | |
public function validateDefinition($element, $objectDefinition = null, JsonPointer $path = null) | ||
{ | ||
foreach ($objectDefinition as $i => $value) { | ||
$property = $this->getProperty($element, $i, new UndefinedConstraint()); | ||
$property = $this->getProperty($element, $i, $this->getFactory()->createInstanceFor('undefined')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good, however, is an unrelated change according to the discussion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @FlorianSW It's a necessary change, the factory passes existing schemaStorage to the new constaint |
||
$definition = $this->getProperty($objectDefinition, $i); | ||
$this->checkUndefined($property, $definition, $path, $i); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it be kinda easier to move
$schemaStorage
to the end of the list? if we'd append it to the existing list, we wouldn't introduce a BC-Break at this point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 This seems like an unnecessary breaking change. However, if this is already a breaking change (and in this core functionality), it probably doesn't make sense to reorder arguments, if it makes sense. Unfortunately, I don't see a good reason to do it here? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steffkes @FlorianSW I think, you're right, it's better to be in the end