Skip to content

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

Merged
merged 28 commits into from
Mar 18, 2024
Merged

Reimplement JsonMapper #69

merged 28 commits into from
Mar 18, 2024

Conversation

reimic
Copy link
Collaborator

@reimic reimic commented Mar 11, 2024

Reimplements parts of the Json Mapper to work in PHP == 7.0 and use less dependencies.

  • Reimplements only the most crucial parts of the jsonmapper/jsonmapper dependency.
  • Reworks Middleware and Handlers into simpler Evaluators. Instead of relying on magic methods for Handlers, a simple loop is run to go through all Evaluators.
  • Reworks Evaluators to auto-configure just before mapping, not during initialization, as was the case prior to this PR. JsonMapper used to resolve all Handlers during initiation, later checking if the resolved values are still available when mapping methods were called. This caused some code to be run during container preparation. This was altered to configure at first map_to_class call, thus no mapper-related code is run before JSON input validation succeeds.
  • Deletes the nonessential Evaluators convenience builder, still preserving elasticity for new Evaluators.
  • Converts Enums to internal lists and strings. Enums were a separate dependency which required PHP > 7.0 and provided little utility. This relates to ScalarTypes and Visibility classes.
  • Joins PropertyMapper, ValueFactory, FactoryRegistry and ScalarCaster into a single streamlined class.
  • Internalizes some small utility methods to reduce class overengineering.
  • Simplifies recursivity.
  • Refactors all code to run with PHP == 7.0.
  • Deletes all implementation parts that evaluated void, mixed , unions and etc. Since this mapper will only ever map to classes that are PHP 7.0 compliant.
  • Adds tests.

@reimic reimic marked this pull request as ready for review March 11, 2024 14:23
@reimic reimic force-pushed the reimpl branch 2 times, most recently from a4951fd to 7415d32 Compare March 11, 2024 16:12
$this->mapper = $mapper;
}

function remove_utf8_bom( $text ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed?

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'll remove remove_utf8_bom() for now.

However there are 2 aspects here. I think each deserves an issue of its own.

  1. 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.
  2. 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 and json_last_error() can be called to get more information on what actually transpired. I've added the most basic Malformed 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.

Copy link
Collaborator

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?

@adamziel
Copy link
Collaborator

adamziel commented Mar 11, 2024

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:

  • Arrays
  • Union types
  • Nullable and missing properties

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
        ]
}

@reimic
Copy link
Collaborator Author

reimic commented Mar 11, 2024

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:
...
Union types

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.

@reimic
Copy link
Collaborator Author

reimic commented Mar 11, 2024

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:
Arrays
Union types
Nullable and missing properties

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.

@adamziel
Copy link
Collaborator

adamziel commented Mar 11, 2024

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.

Types are expressed either as type annotations or as comments. Union types are all over Blueprints, e.g. ResourceInterface|string, they're just stored in comments since PHP 7.0 doesn't support them. Ditto for typed collections like string[].

@adamziel
Copy link
Collaborator

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.

reimic added 2 commits March 12, 2024 12:01
- 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
@reimic
Copy link
Collaborator Author

reimic commented Mar 12, 2024

@reimic the PR description covers a delta between the JsonMapper library and this PR. It would be more useful if it instead discussed the problem being solved, gave a short overview of the solution, and provided testing instructions.

Fair point. I'll do one, after all threads are resolved.

@reimic reimic requested a review from adamziel March 16, 2024 15:12
Copy link
Collaborator

@adamziel adamziel left a 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?

@adamziel adamziel merged commit 62d460c into WordPress:trunk Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants