-
Notifications
You must be signed in to change notification settings - Fork 10
Reimplement JsonMapper #69
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
Conversation
a4951fd
to
7415d32
Compare
$this->mapper = $mapper; | ||
} | ||
|
||
function remove_utf8_bom( $text ) { |
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.
Why is this needed?
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.
I'll remove remove_utf8_bom()
for now.
However there are 2 aspects here. I think each deserves an issue of its own.
- I've encountered multiple problems with
json_decode()
. It was very sensitive to a bunch of things related to Windows end line characters and general formatting. This should be looked at as a semantically and syntactically valid JSON might be provided, but some miniscule formatting issues will cause it to fail at decoding. Should be reviewed before v1. - Despite the above. It will always be possible that the JSON will me in some shape or form syntactically malformed. In such cases
json_decode()
returns null andjson_last_error()
can be called to get more information on what actually transpired. I've added the most basicMalformed JSON.
message, just to signify the issue was syntactic, not semantic in nature. But I know from experience this is one of the most frustrating parts of using a new API. If we truly aspire to deliver a premium way of setting up/developing WordPress, providing a robust troubleshooting experience is paramount. That said, this is not a v1 deployment-blocking issue.
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.
Would you please start a new issue with the problem description and reproduction steps?
I left some notes, but things are mostly looking good. Yay! Thank you for you great work on this! I'd love to also see some more tests around:
For example, the following Blueprint should be correctly mapped even though the array contains two different data types: {
"plugins": [
"https://downloads.wordpress.org/plugin/wordpress-importer.zip",
{ "resource": "url", "url": "https://mysite.com" }
]
} However, this Blueprint should be rejected since numbers are not valid as a plugin definition: {
"plugins": [
"https://downloads.wordpress.org/plugin/wordpress-importer.zip",
123
]
} |
I do not see how union types play into this - they are not a PHP 7.0 thing. I would imagine we should just not use them anywhere in the lib. |
Most definitely. I do have quite a lot of tests in mind. How would you like to progress from here: test the mapper now or merge it, finish other downgrade-related matters and than circle back? I'd recommend the latter. I think, and this is also reflected in your comments, there are still some things we can iterate upon. A testing-focused PR done in a week or so would be a great occasion to go through everything and make some improvements with "fresh" eyes. |
Types are expressed either as type annotations or as comments. Union types are all over Blueprints, e.g. |
Let's test the mapper now to make sure it actually works. It's a central, critical part of the system. Breaking it would generate more work with debugging, reverting this or etc. |
- generalizes Json Mapper - reworks tests to test the proper configuration of the BlueprintMapper as an instance of the generic JsonMapper - reworks tests to test generic JsonMapper features independently of custom configuration
Fair point. I'll do one, after all threads are resolved. |
src/WordPress/Blueprints/Model/DataClass/ActivatePluginStep.php
Outdated
Show resolved
Hide resolved
… already null by default
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.
Let's get this in! @reimic would you please update the description for any future readers as discussed earlier?
Reimplements parts of the Json Mapper to work in PHP == 7.0 and use less dependencies.
jsonmapper/jsonmapper
dependency.Middleware
andHandlers
into simplerEvaluators
. Instead of relying on magic methods for Handlers, a simple loop is run to go through all Evaluators.map_to_class
call, thus no mapper-related code is run before JSON input validation succeeds.ScalarTypes
andVisibility
classes.PropertyMapper
,ValueFactory
,FactoryRegistry
andScalarCaster
into a single streamlined class.void
,mixed
, unions and etc. Since this mapper will only ever map to classes that are PHP 7.0 compliant.