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

Don't try parse value as string if it's already enum unit. PhpEnumType #1481

Closed
wants to merge 2 commits into from

Conversation

danbka33
Copy link

Fixes a bug in subscriptions when an enum object is received instead of a string

I added a couple of tests to check how the value is parsed.

In addition, I fix a test that fails depending on the PHP version (tests/Type/EnumTypeTest.php@testAllowsSimpleArrayAsValues). It might be worth using a check that does not include things that may change depending on the php version.

All test passed. Tested with PHP 8.2.10 (cli) (built: Aug 31 2023 18:52:55) (NTS)

изображение

If we talk about the backward compatibility of these changes, then nothing should break.

@danbka33 danbka33 changed the title Don't try parse value as string if it's already enum unit Don't try parse value as string if it's already enum unit. PhpEnumType Nov 18, 2023
@shmax
Copy link
Contributor

shmax commented Nov 18, 2023

Not sure I totally follow what you're up to, here, but if it makes any difference there's a PR pending that refactors the PHPEnumType constructor to take a config object instead of a string:

#1375

@danbka33
Copy link
Author

I'm sure there won't be any problems with this. I saw this PR. Only one method from the parent class is added here. I’m sure that the merger with your PR will take place without conflicts, but even if there are conflicts, it can be solved.

@@ -9,6 +9,10 @@ You can find and compare releases at the [GitHub release page](https://github.co

## Unreleased

### Fixed

- Fix PhpEnumType parse value if it's already enum value https://github.com/webonyx/graphql-php/pull/1481
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Fix PhpEnumType parse value if it's already enum value https://github.com/webonyx/graphql-php/pull/1481
- Handle enum values in `PhpEnumType::parseValue()` https://github.com/webonyx/graphql-php/pull/1481

@spawnia spawnia added the bug label Dec 5, 2023
'extensions' => [
'debugMessage' => 'Expected a value of type SimpleEnum but received: "WRONG". Cannot serialize value as enum: "WRONG"',
'trace' => [
['call' => 'GraphQL\Type\Definition\EnumType::serialize()'],
['call' => 'GraphQL\Type\Definition\EnumType::serialize(\'WRONG\')'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately, this fails in lower PHP versions - see https://github.com/webonyx/graphql-php/actions/runs/6914711109/job/19325221423?pr=1481. Not sure why it failed for you anyways, we are testing on PHP 8.2 in CI?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants