Conversation
a4951fd to
7415d32
Compare
| $this->mapper = $mapper; | ||
| } | ||
|
|
||
| function remove_utf8_bom( $text ) { |
There was a problem hiding this comment.
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.
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
Reimplements parts of the Json Mapper to work in PHP == 7.0 and use less dependencies.
jsonmapper/jsonmapperdependency.MiddlewareandHandlersinto simplerEvaluators. Instead of relying on magic methods for Handlers, a simple loop is run to go through all Evaluators.map_to_classcall, thus no mapper-related code is run before JSON input validation succeeds.ScalarTypesandVisibilityclasses.PropertyMapper,ValueFactory,FactoryRegistryandScalarCasterinto a single streamlined class.void,mixed, unions and etc. Since this mapper will only ever map to classes that are PHP 7.0 compliant.