Skip to content

fix(field-resolver): re-add support for isser methods #720

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 4 commits into from
Aug 13, 2020

Conversation

Kocal
Copy link
Contributor

@Kocal Kocal commented Aug 7, 2020

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? yes/no
Tests pass? yes
Documented? yes
Fixed tickets #...
License MIT

I'm updating from 0.11 to 0.13 on our big app at work, and we have a lot of errors on fields which depend of isser methods.

Isser methods support has been removed in 0.13, because the Symfony PropertyAccessor has been removed too, due to performance issue for large schema.

However, I think it can be nice to re-add a basic support out-of-the-box for issers.

What do you think?
Thanks!

@Kocal Kocal changed the base branch from master to 0.13 August 7, 2020 10:43
@Kocal Kocal requested a review from mcg-web August 7, 2020 10:48
Copy link
Contributor

@mcg-web mcg-web left a comment

Choose a reason for hiding this comment

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

Thank you @Kocal

@ruudk
Copy link
Contributor

ruudk commented Aug 13, 2020

Will it work when the field is called isEnabled and the method is called function isEnabled() : bool? By looking at the code, I expect it will try to fetch it as property but I could be wrong.

Copy link
Contributor

@mcg-web mcg-web left a comment

Choose a reason for hiding this comment

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

some PHP CS fix required

@mcg-web
Copy link
Contributor

mcg-web commented Aug 13, 2020

@ruudk yes because we test isser before public property.

@ruudk
Copy link
Contributor

ruudk commented Aug 13, 2020

@mcg-web Maybe this explain it better: https://3v4l.org/tfb8l

@ruudk
Copy link
Contributor

ruudk commented Aug 13, 2020

Maybe a fix could be to add another elseif like this:

} elseif (null !== $getter = self::guessObjectMethod($objectOrArray, $fieldName, '')) { // prefix is empty so it will try `isValid()`
    $value = $objectOrArray->$getter();
}

@mcg-web
Copy link
Contributor

mcg-web commented Aug 13, 2020

Maybe the best fix to this issue is to add an optional ResolverPropertyAccess that required PropertyAccess. So your both projects will be sure to work like be before 0.13. I want to keep the default as simple as possible, I don't want to rewrite PropertyAccess component here. Can this satisfy both of you @Kocal and @ruudk?

@ruudk
Copy link
Contributor

ruudk commented Aug 13, 2020

Hmm, then I'd rather have this PR merged, and I will fix these "special" cases on my side.... feels like a missing feature to me but don't want to further complicate GraphQLBundle project with having to support 2 types of property access resolvers.

@Kocal
Copy link
Contributor Author

Kocal commented Aug 13, 2020

Both options are fine for me

@mcg-web
Copy link
Contributor

mcg-web commented Aug 13, 2020

Ok, I'll merge this one! anyway you can use config to define your custom default field resolver

overblog_graphql:
    definitions:
         default_field_resolver: my_custom_service_id

@mcg-web mcg-web merged commit c6107c3 into overblog:0.13 Aug 13, 2020
@ruudk
Copy link
Contributor

ruudk commented Aug 13, 2020

Thanks for merging. Is it too soon to ask for a tagged release 🙈 😂 ?

@Kocal Kocal deleted the re-add-isser-support branch August 13, 2020 09:48
@mcg-web
Copy link
Contributor

mcg-web commented Aug 13, 2020

Maybe a fix could be to add another elseif like this:

} elseif (null !== $getter = self::guessObjectMethod($objectOrArray, $fieldName, '')) { // prefix is empty so it will try `isValid()`
    $value = $objectOrArray->$getter();
}

@ruudk I'm going also to (re-)introduce before release since it is also present on master.

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.

3 participants