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.

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

Improve no property behavior and schema storage #6

Merged
merged 6 commits into from
Nov 20, 2020
Merged

Conversation

dafeder
Copy link
Member

@dafeder dafeder commented Nov 18, 2020

Fixes two issues:

  1. It was hard to tell if a property was actually set. Now, requesting a non-existant property will return null instead of false, and the magic __isset() method has been added.
  2. We were storing the schema as an Opis Schema object, but that's not very useful because it doesn't have any way to convert back into a string. Now we just store the string and instantiate a Schema object internally where needed.

@@ -34,7 +34,9 @@ public function __construct(string $json = "{}", string $schema = "{}")
throw new \InvalidArgumentException("Invalid JSON: " . json_last_error_msg());
}

$this->schema = Schema::fromJsonString($schema);
if (Schema::fromJsonString($schema)) {
$this->schema = $schema;
Copy link
Member Author

Choose a reason for hiding this comment

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

Occurs to me, another option to storing the schema as a string would be to store it as another JsonObject...

Copy link
Member Author

Choose a reason for hiding this comment

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

Or on second thought, maybe as an decoded json object? That way the validator doesn't need to re-decode it every time, which is probably a bit expensive, althought, not THAT expensive. Probably fine as a string for now? Could go either way.

@fmizzell fmizzell merged commit cf5d492 into master Nov 20, 2020
@fmizzell fmizzell deleted the noproperty branch November 20, 2020 19:07
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.

2 participants