Skip to content

php merge for associative array non-integer intexed#98

Merged
vladar merged 6 commits intowebonyx:masterfrom
decebal:master
Mar 16, 2017
Merged

php merge for associative array non-integer intexed#98
vladar merged 6 commits intowebonyx:masterfrom
decebal:master

Conversation

@decebal
Copy link
Copy Markdown
Contributor

@decebal decebal commented Mar 16, 2017

+= is useful as an array merge just in case the array is numerically indexed, which is not the case here at all.

Problems solved by this:

fragment Fb on User {
  id
  _posts2R94ZT: posts(first: $first_1) {
    totalCount
  }
}

fragment Fc on User {
  _posts2R94ZT: posts(first: $first_1) {
    edges {
      node {
        id
        ...F5
      }
      cursor
    }
    pageInfo {
      hasNextPage
      hasPreviousPage
    }
  }
  id
  ...Fb
}

fragment selection would be merged as opposed to first fragment selection being ignored before.

@vladar
Copy link
Copy Markdown
Member

vladar commented Mar 16, 2017

Actually += is mostly useful for associative arrays vs int-indexed arrays. Can you please update the PR with PHPUnit test case which you are trying to fix? As it is not clear from this PR what bug it actually tries to fix?

This is a test class for ResolveInfo which you can update: https://github.com/webonyx/graphql-php/blob/master/tests/Type/ResolveInfoTest.php

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 93.717% when pulling 51e67d4 on decebal:master into f77bd17 on webonyx:master.

@decebal
Copy link
Copy Markdown
Contributor Author

decebal commented Mar 16, 2017

@vladar I am not sure what you have in mind when stating += is mostly useful. The effects of it are reflected better on this thread . In my app, useful means no loss of selected fields as opposed to maybe array merging performance. One other way of improving the merge performance and keep the same merging method would be to change the structure of selected fields by using as keys global integers to identify types as opposed to what currently is defined as [type_name] => true, but that can prove ineffective for other purposes.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 93.717% when pulling ca92ae4 on decebal:master into f77bd17 on webonyx:master.

@vladar
Copy link
Copy Markdown
Member

vladar commented Mar 16, 2017

@decebal, now I see what you mean. The issue occurs when two fragments request the same field with different sub-selection. Current solution would only include selection of first fragment visited, while we actually need to merge them all.

Thanks for noticing and fixing! Please let me know when you are done with PR, I'll merge it.

@decebal
Copy link
Copy Markdown
Contributor Author

decebal commented Mar 16, 2017

@vladar it's done now, unless there is something else I can relate to this, thanks

@vladar vladar merged commit 7ef9f91 into webonyx:master Mar 16, 2017
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