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

centralize errors #364

Merged
merged 4 commits into from
Mar 7, 2017
Merged

centralize errors #364

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
20f45e6
centralize errors
shmax Feb 26, 2017
b370d67
isolate 'more' info
shmax Feb 26, 2017
be50388
throw exception for missing error message
shmax Mar 4, 2017
d05f90c
swap args
shmax Mar 4, 2017
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
Prev Previous commit
Next Next commit
throw exception for missing error message
  • Loading branch information
@shmax
shmax committed Mar 4, 2017
commit be50388bee73604559ed980a1c4e6e6aaa2a038a
6 changes: 5 additions & 1 deletion src/JsonSchema/ConstraintError.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ public function getMessage()
self::UNIQUE_ITEMS => 'There are no duplicates allowed in the array'
);

return isset($messages[$name]) ? $messages[$name] : "Unknown Error: $name";
if (!isset($messages[$name])) {
throw new InvalidArgumentException('Missing error message for ' . $name);
}

return $messages[$name];
}
}
6 changes: 4 additions & 2 deletions src/JsonSchema/Constraints/BaseConstraint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,20 @@ public function __construct(Factory $factory = null)

public function addError(JsonPointer $path = null, ConstraintError $constraint = null, array $more = array())
{
$message = $constraint ? $constraint->getMessage() : '';
$name = $constraint ? $constraint->getValue() : '';
$error = array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than sticking with the array format, why not roll all this into the ConstraintError class? That way you can just pass one object around, and it doesn't remain an unspecced property bag the way it is now. More reliable for anyone needing to interact with it and easier to extend. This is the sort of thing I was intending to do when I was going to overhaul the errors, but you beat me to it with this PR ;-).

Copy link
Collaborator Author

@shmax shmax Mar 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

ConstraintError is just an enum. I'm not sure it's really meant to be built up into a proper class, with its own members and whatnot. You don't new them, for example. You can read up on it here: https://github.com/marc-mabe/php-enum

Copy link
Collaborator Author

@shmax shmax Mar 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

As mentioned in the other comment, ConstraintError is not used like a conventional class. It's an enum, with a very rigid use pattern. We could throw it out and use a regular class like you're suggesting, but we would still have a lot of fragmentation of the properties passed in (although it's a little tighter now that I'm sectioning more off into the params bucket). I'm not sure that using a class would really gain us anything other than sort of formalizing the values it manages, but this is the only place in the code where the values are gathered together, so what would be the gain? A consumer would now have a class instance dropped in his lap to figure out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would quite like a class, but I can also see where you're coming from here too - I think we had quite different visions for how the errors should eventually end up working.

May I think on this a bit? I like what you're doing here, but I would like to see if I can think of a way to integrate the two approaches, as I see no reason why they can't work together.

Would you consider accepting a PR on your PR branch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was not trying to solve the error system, here. You are still welcome to do that. I was only trying to get localizers unstuck. Why don't we just evaluate this PR in light of that goal, merge it into 6.0 if it accomplishes what it sets out to do, and then you can revamp the error system in another PR all you like and no argument from me (although I will have plenty of arguments)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me :-)

'property' => $this->convertJsonPointerIntoPropertyPath($path ?: new JsonPointer('')),
'pointer' => ltrim(strval($path ?: new JsonPointer('')), '#'),
'message' => ucfirst(vsprintf($constraint->getMessage(), array_map(function ($val) {
'message' => ucfirst(vsprintf($message, array_map(function ($val) {
if (is_scalar($val)) {
return $val;
}

return json_encode($val);
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of json-encoding complex types. Good idea 👍.

}, array_values($more)))),
'constraint' => array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above - if you feed it to the constructor, then this becomes unnecessary.

'name' => $constraint ? $constraint->getValue() : '',
'name' => $name,
'params' => $more
)
);
Expand Down
2 changes: 1 addition & 1 deletion src/JsonSchema/Constraints/ConstraintInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public function addErrors(array $errors);
/**
* adds an error
*
* @param JsonPointer|null $path
* @param JsonPointer |null $path
* @param ConstraintError|null $constraint the constraint/rule that is broken, e.g.: ConstraintErrors::LENGTH_MIN()
* @param array $more more array elements to add to the error
*/
Expand Down