Skip to content
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.

Sign up for GitHub

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

Return points, spelling and dead code #304

Merged
merged 5 commits into from
Oct 3, 2016
Merged

Return points, spelling and dead code #304

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0de7ae8
Clarity and consistency of return points
calcinai Aug 23, 2016
bda5676
Spelling
calcinai Aug 23, 2016
240a8e0
Removed unused declarations and vars
calcinai Aug 23, 2016
37aa80a
Updated tests
calcinai Aug 23, 2016
93d8056
Reverted return point change and updated test
calcinai Oct 3, 2016
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
1 change: 1 addition & 0 deletions bin/validate-json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ function getUrlFromPath($path)
/**
* Take a HTTP header value and split it up into parts.
*
* @param $headerValue
* @return array Key "_value" contains the main value, all others
* as given in the header value
*/
Expand Down
1 change: 0 additions & 1 deletion src/JsonSchema/Constraints/EnumConstraint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
*/

namespace JsonSchema\Constraints;
use JsonSchema\Validator;
use JsonSchema\Entity\JsonPointer;

/**
Expand Down
2 changes: 1 addition & 1 deletion src/JsonSchema/Constraints/ObjectConstraint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ public function validateElement($element, $matches, $objectDefinition = null, Js
*
* @param \stdClass $element Element to validate
* @param \stdClass $objectDefinition ObjectConstraint definition
* @param JsoinPointer|null $path Path?
* @param JsonPointer|null $path Path?
*/
public function validateDefinition($element, $objectDefinition = null, JsonPointer $path = null)
{
Expand Down
7 changes: 4 additions & 3 deletions src/JsonSchema/Constraints/TypeConstraint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,9 @@ public function check($value = null, $schema = null, JsonPointer $path = null, $
*
* @param mixed $value Value to validate
* @param array $type TypeConstraints to check agains
* @param array $wording An array of wordings of the valid types of the array $type
* @param array $validTypesWording An array of wordings of the valid types of the array $type
* @param boolean $isValid The current validation value
* @param $path
*/
protected function validateTypesArray($value, array $type, &$validTypesWording, &$isValid,
$path) {
Expand Down Expand Up @@ -111,10 +112,10 @@ protected function validateTypesArray($value, array $type, &$validTypesWording,
*/
protected function implodeWith(array $elements, $delimiter = ', ', $listEnd = false) {
if ($listEnd === false || !isset($elements[1])) {
return implode(', ', $elements);
return implode($delimiter, $elements);
}
$lastElement = array_slice($elements, -1);
$firsElements = join(', ', array_slice($elements, 0, -1));
$firsElements = join($delimiter, array_slice($elements, 0, -1));
$implodedElements = array_merge(array($firsElements), $lastElement);
return join(" $listEnd ", $implodedElements);
}
Expand Down
2 changes: 1 addition & 1 deletion src/JsonSchema/Uri/Retrievers/AbstractRetriever.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

/**
* AbstractRetriever implements the default shared behavior
* that all decendant Retrievers should inherit
* that all descendant Retrievers should inherit
* @author Steven Garcia <[email protected]>
*/
abstract class AbstractRetriever implements UriRetrieverInterface
Expand Down
3 changes: 2 additions & 1 deletion src/JsonSchema/Uri/Retrievers/FileGetContents.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
namespace JsonSchema\Uri\Retrievers;

use JsonSchema\Exception\ResourceNotFoundException;
use JsonSchema\Validator;

/**
* Tries to retrieve JSON schemas from a URI using file_get_contents()
Expand Down Expand Up @@ -83,5 +82,7 @@ protected static function getContentTypeMatchInHeader($header)
if (0 < preg_match("/Content-Type:(\V*)/ims", $header, $match)) {
return trim($match[1]);
}

return null;
}
}
6 changes: 3 additions & 3 deletions src/JsonSchema/Uri/UriRetriever.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,19 +40,19 @@ class UriRetriever implements BaseUriRetrieverInterface
*
* @param UriRetrieverInterface $uriRetriever
* @param string $uri
* @return bool|void
* @return bool
*/
public function confirmMediaType($uriRetriever, $uri)
{
$contentType = $uriRetriever->getContentType();

if (is_null($contentType)) {
// Well, we didn't get an invalid one
return;
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure, how many users uses this method, but changing from null to a boolean value could be problematic, especially in code something like:

if ( confirmMediaType( $uriRetriever, $uri ) ) {
  // the code that runs when true
} else {
  // the former "null" value-code
}

It now would break, as null evaluates to false, and not to true. So in some cases, this could be a problem. However, the new values makes sense, too. I'm really not sure :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that (in master) the function could return true when it's a schema from json-schema.org, and null if it is explicitly valid, it just isn't seem all that logical. The only usage internally just relies on it halting with an exception if it's invalid.

Is it even feasible that someone has written code to handle the different return types? If you're using the library for validation, it's going to be called again anyway and you won't have any control of the result..

That said, maybe there is an argument for leaving it as it is, because the return value is basically irrelevant anyway!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@FlorianSW thank you so much for your feedback. This is exactly my concern.

As you pointed out, null is falsy. Perhaps returning false here is the solution as it would be compliant with the bool return type and also be compatible with previous control-flow usage, although that would not be true for === null or is_null comparisons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving forward with this, should I just revert the section in question?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maintaining BC is preferred.

}

if (in_array($contentType, array(Validator::SCHEMA_MEDIA_TYPE, 'application/json'))) {
return;
return true;
}

if (substr($uri, 0, 23) == 'http://json-schema.org/') {
Expand Down
1 change: 0 additions & 1 deletion src/JsonSchema/Validator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

namespace JsonSchema;

use JsonSchema\Constraints\SchemaConstraint;
use JsonSchema\Constraints\Constraint;
use JsonSchema\Entity\JsonPointer;

Expand Down
4 changes: 2 additions & 2 deletions tests/Uri/UriRetrieverTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ public function testConfirmMediaTypeAcceptsJsonSchemaType()
->method('getContentType')
->will($this->returnValue('application/schema+json'));

$this->assertEquals(null, $retriever->confirmMediaType($retriever, null));
$this->assertEquals(true, $retriever->confirmMediaType($retriever, null));
}

public function testConfirmMediaTypeAcceptsJsonType()
Expand All @@ -253,7 +253,7 @@ public function testConfirmMediaTypeAcceptsJsonType()
->method('getContentType')
->will($this->returnValue('application/json'));

$this->assertEquals(null, $retriever->confirmMediaType($retriever, null));
$this->assertEquals(true, $retriever->confirmMediaType($retriever, null));
}

/**
Expand Down