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

add queryplan #436

Merged
merged 1 commit into from Mar 9, 2019

Conversation

Projects
None yet
4 participants
@keulinho
Copy link
Contributor

commented Feb 1, 2019

Provide the possibility to lookahead the Query Execution.
This one adds the API proposed in #65.
From #65:

  1. What sub-fields were requested (regardless of fragment types)? -> still possible
  2. What types were referenced by fragments? -> could be added
  3. What sub-fields were requested for given type? -> possible
  4. What args/directives were set for given field? -> possible for args, directives could be added

@keulinho keulinho force-pushed the keulinho:add-queryplan branch from 515ba09 to 7b787b5 Feb 1, 2019

self::assertEquals($expectedReferencedFields, $queryPlan->referencedFields());
self::assertEquals(['url', 'width', 'height'], $queryPlan->subFields('Image'));
static::assertTrue($queryPlan->hasField('url'));

This comment has been minimized.

Copy link
@mfn

mfn Feb 1, 2019

What's up with the self:: vs. static:: calls here?

Maybe it's through out the codebase and I just didn't notice it before 🤔

This comment has been minimized.

Copy link
@keulinho

keulinho Feb 1, 2019

Author Contributor

good catch 👍
the convention seems to be self:: but i'm used to static::

fixed it

@keulinho keulinho force-pushed the keulinho:add-queryplan branch 2 times, most recently from 85bdb8b to 3e8d848 Feb 1, 2019

* @param mixed[] $variableValues
* @param FragmentDefinitionNode[] $fragments
*
* @throws Error

This comment has been minimized.

Copy link
@simPod

simPod Feb 1, 2019

Contributor

IMO the @throws anotation can be dropped. I have never found a way how to sustainably maintain those across the whole project.

public function hasType(string $type) : bool
{
return count(array_filter($this->referencedTypes(), static function ($element) use ($type) {

This comment has been minimized.

Copy link
@simPod

simPod Feb 1, 2019

Contributor

I guess we can typehint string $element. And then change variable name to $referencedType

/**
* @return string[]
*/
public function referencedTypes() : array

This comment has been minimized.

Copy link
@simPod

simPod Feb 1, 2019

Contributor

I'd prefix with get

/**
* @return string[]
*/
public function referencedFields() : array

This comment has been minimized.

Copy link
@simPod

simPod Feb 1, 2019

Contributor

prefix get as well.

public function hasField(string $field) : bool
{
return count(array_filter($this->referencedFields(), static function ($element) use ($field) {

This comment has been minimized.

Copy link
@simPod

simPod Feb 1, 2019

Contributor
Suggested change
return count(array_filter($this->referencedFields(), static function ($element) use ($field) {
return count(array_filter($this->referencedFields(), static function (string $referencedField) use ($field) {
} elseif ($selectionNode instanceof FragmentSpreadNode) {
$spreadName = $selectionNode->name->value;
if (isset($this->fragments[$spreadName])) {
/** @var FragmentDefinitionNode $fragment */

This comment has been minimized.

Copy link
@simPod

simPod Feb 1, 2019

Contributor

this can be dropped, no?

@@ -99,6 +99,13 @@ class ResolveInfo
*/
public $variableValues = [];
/**
* @internal

This comment has been minimized.

Copy link
@simPod

simPod Feb 1, 2019

Contributor

the var is private so I guess we can drop this

*
* @var QueryPlan
*/
private $queryPlan = null;

This comment has been minimized.

Copy link
@simPod

simPod Feb 1, 2019

Contributor

null is default, can be dropped

@@ -179,6 +186,22 @@ public function getFieldSelection($depth = 0)
return $fields;
}
public function lookahead() : QueryPlan

This comment has been minimized.

Copy link
@simPod

simPod Feb 1, 2019

Contributor

If meant as verb, the proper camel case is lookAhead

use GraphQL\Type\Schema;
use PHPUnit\Framework\TestCase;
class QueryPlanTest extends TestCase

This comment has been minimized.

Copy link
@simPod

simPod Feb 1, 2019

Contributor
Suggested change
class QueryPlanTest extends TestCase
final class QueryPlanTest extends TestCase

@keulinho keulinho force-pushed the keulinho:add-queryplan branch from 3e8d848 to 5b3f44c Feb 1, 2019

@vladar

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

This is great! I need some time to digest it before I can post any feedback or merge. Thanks!

],
array_keys((array) $info)
);

This comment has been minimized.

Copy link
@vladar

vladar Feb 19, 2019

Member

Why deleting this? Can't you just add queryPlan key to this list?

This comment has been minimized.

Copy link
@keulinho

keulinho Feb 26, 2019

Author Contributor

queryPlan is declared private because it should only be loaded lazy when you call lookAhead()
When you cast the resolveInfo to array now it cpontains some binary string instead of queryPlan because it is private.
Furthermore i really saw no value added by this test, because every key mentionied there would be used for own assertions, so the test will fail anyway if you remove one of the public properties.

This comment has been minimized.

Copy link
@vladar

vladar Mar 6, 2019

Member

Sorry, missed your reply. Makes sense.

@vladar vladar merged commit 29eba82 into webonyx:master Mar 9, 2019

2 checks passed

Scrutinizer Analysis: 109 new issues, 16 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.