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

fix: handle arrays before casting when normalizing attribute values #220

Merged
merged 4 commits into from
Apr 8, 2024

Conversation

justlevine
Copy link
Contributor

What

This PR fixes the logic inside Block::normalize_attribute_values() to:

  • return array values before casting to the normalized type.
  • cast strings to an array if the $type is an array, but the $value is not.

No effort has been made to determine the possible Scalar types that $value can accept, and instead is assumed to only be a string|array as currently defined in the doc-block.

Why

You can't cast arrays to strings in PHP.

Additional notes

Important

At this stage, we're likely going to start seeing merge conflicts on phpstan-baseline.neon, due to multiple lines getting deleted at the source.

To resolve without waiting for @justlevine , restore the base (i.e. main branch) phpstan-baseline.neon file, and then reapply the removal of the specific allow-listed error from the phpstan-baseline.neon diff in this PR.

@justlevine justlevine requested a review from a team as a code owner March 29, 2024 09:54
Copy link

changeset-bot bot commented Mar 29, 2024

🦋 Changeset detected

Latest commit: 509e25c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@wpengine/wp-graphql-content-blocks Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment on lines +299 to +301
if ( is_array( $value ) ) {
return $value;
}
Copy link

Choose a reason for hiding this comment

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

I think it would be clearer to move this into the array case block, and if $type is not array, we shouldn't return an array, right?

Copy link
Contributor Author

@justlevine justlevine Mar 29, 2024

Choose a reason for hiding this comment

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

It's two separate guards.

This guard covers if the value is an array. If so we can't cast it to anything so we need to return early (before our switch) regardless of the expected type. (Long term the early return would be replaced with a nested validator based on the attribute source type).

The case 'array' guard is to handle where the attribute type is defined as an array, but the value isn't an array. This uncommon edge case otherwise results in a breaking GraphQL error when resolving, (since 'list_of' requires an array format for even a single value).

theodesp
theodesp previously approved these changes Apr 4, 2024
@theodesp theodesp merged commit e348494 into wpengine:main Apr 8, 2024
5 of 6 checks passed
@justlevine justlevine deleted the fix/dont-cast-arrays-as-strings branch April 8, 2024 09:32
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.

None yet

3 participants