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

Extend complex types #66

Conversation

Zamfi99
Copy link
Contributor

@Zamfi99 Zamfi99 commented Jul 10, 2024

Description

The current implementation of strval results in the loss of type information for non-string values. This transformation can lead to unexpected behavior in front-end (FE) applications and may disrupt existing contracts.

The usage of strval on non-string values results in the following conversions:

  • strval(false) results in ""
  • strval(true) results in "1"
  • strval(5) results in "5"

Such conversions can alter the original types, potentially causing unforeseen issues in FE applications that rely on precise data types.

This PR introduces a change to encode non-string types as JSON strings. This approach ensures that the original types can be accurately decoded and maintained in FE applications, thereby preserving the expected behavior and integrity of existing contracts.

Steps to Test

  1. Check out PR.
  2. Run composer run test.

@Zamfi99 Zamfi99 requested a review from a team as a code owner July 10, 2024 01:38
@alecgeatches
Copy link
Contributor

@Zamfi99 Thank you! This looks like a good idea, especially fixing the boolean examples you gave. We'd like to merge this soon and mostly need to consider possible API version updates to account for the changes in data representation.

@alecgeatches alecgeatches self-assigned this Jul 24, 2024
@alecgeatches
Copy link
Contributor

@Zamfi99 I wanted to add some more tests for other scalar types, so I pushed a couple of changes to your PR fork: Zamfi99#1. PRing to a fork is not a typical workflow for me, but I think it makes sense here. If you can approve those changes to your fork, we're all set to merge in this PR. If it takes a bit longer, I'll probably migrate these changes to a new branch and merge that instead.

Thank you for your time and improvements to the plugin!

…types

Add additional tests to extend complex types
@alecgeatches alecgeatches changed the base branch from trunk to planned-release/1.3.0 July 25, 2024 15:47
@alecgeatches
Copy link
Contributor

@Zamfi99 Merging this in to planned-release/1.3.0, and planning to address your other open PRs (#65, #67) before making a release next week. Thanks a ton for your time so far!

@alecgeatches alecgeatches merged commit 184bca0 into Automattic:planned-release/1.3.0 Jul 26, 2024
3 checks passed
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