Skip to content

Conversation

bartv2
Copy link
Contributor

@bartv2 bartv2 commented Aug 16, 2020

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Documented? yes
Fixed tickets #...
License MIT
  • Skip properties without a Field annotation
  • Enable field type guessing for Input annotation

@mcg-web mcg-web requested a review from Vincz August 16, 2020 21:00
@bartv2 bartv2 marked this pull request as draft August 16, 2020 22:00
@bartv2 bartv2 marked this pull request as ready for review August 16, 2020 22:50
Copy link
Collaborator

@Vincz Vincz left a comment

Choose a reason for hiding this comment

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

Looks good, but we should add more tests.
Also, wait for #738 & #740 to be merged.
And at the moment, the guessType doesn't handle typed property (it only check for doctrine annotations). We should first check for typing. Example:

classe MyType {
    /**
      * @GQL\Field
      * @ORM\Column(type="boolean")
      */
    protected int $variable;
}

In this case, we should guess the type as int and not boolean.
Would you mind to add this logic in this PR too ?

$fieldAnnotation = self::getFirstAnnotationMatching($annotations, GQL\Field::class);

// No field annotation found
if (!$fieldAnnotation) {
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
if (!$fieldAnnotation) {
if (null === $fieldAnnotation) {

// Ignore field with resolver when the type is an Input
if (isset($fieldAnnotation->resolve)) {
return [];
}

$fieldName = $reflector->getName();
$fieldType = $fieldAnnotation->type;
$fieldType = $fieldAnnotation->type ?? null;
if (!$fieldType) {
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
if (!$fieldType) {
if (!isset($fieldAnnotation->type)) {

@bartv2 bartv2 requested a review from Vincz August 17, 2020 12:28
@Vincz
Copy link
Collaborator

Vincz commented Aug 17, 2020

Good job @bartv2 !
Can you do the same in getTypeFieldConfigurationFromReflector for regular GraphQL types ?
Trying to get the type from type hint first ?

@bartv2
Copy link
Contributor Author

bartv2 commented Aug 17, 2020

Something like this?

@Vincz
Copy link
Collaborator

Vincz commented Aug 18, 2020

@bartv2 Yes ! Looks good ! Just run a little composer fix-cs and update the doc about auto-guessing and we are good to go :-)

Copy link
Collaborator

@Vincz Vincz left a comment

Choose a reason for hiding this comment

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

Awesome. Thank you !

@Vincz Vincz merged commit 04818f2 into overblog:master Aug 18, 2020
@bartv2 bartv2 deleted the patch-2 branch August 18, 2020 18:58
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