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

Made it possible for totalCount to be a promise #782

Closed
wants to merge 1 commit into from
Closed

Made it possible for totalCount to be a promise #782

wants to merge 1 commit into from

Conversation

rsolyanik
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Documented? no
License MIT

@mcg-web
Copy link
Member

mcg-web commented Nov 11, 2020

Sorry but all released versions are feature freeze. We only accept bugfix or security fix right now for these branches. Your feature add bc so can't definitely not be accepted.

@mcg-web mcg-web closed this Nov 11, 2020
@rsolyanik
Copy link
Contributor Author

rsolyanik commented Nov 11, 2020

Sorry but all released versions are feature freeze. We only accept bugfix or security fix right now for these branches. Your feature add bc so can't definitely not be accepted.

@mcg-web this don't break bc, but fixes error Object of class GraphQL\\Executor\\Promise\\Promise could not be converted to int when trying load total counts in nested lists via data loaders for such queries as{ customers(first: 1000) { edges { node { id, loyaltyAccounts { totalCount edges { node { .... } } } } } } }
#286

@mcg-web
Copy link
Member

mcg-web commented Nov 11, 2020

Changing the signature of an interface is a bc. If the fix introduce any bc it can't be allowed on this branch maybe it can be fixed a different way?

@murtukov
Copy link
Member

murtukov commented Nov 11, 2020

@mcg-web Well, that's a tricky case. What if fixing a bug requires changing the signature of a function? Should we now just leave bugs only because of it? And is it a new feature anyway? I thinks it was supposed to be able to return promises from the beginning and if it doesn't work as supposed, then it's a bug.

What is a BC break? It's something that makes your code fail. The following signature:

public function getTotalCount()

is a superset of:

public function getTotalCount(): ?int

How can it possibly make your code fail? Maybe only if someone decides to use the method reflection, but I don't see any reson to do so.

@mcg-web
Copy link
Member

mcg-web commented Nov 11, 2020

@murtukov there's 2 methods changed, the getter doesn't effectively break anything but what about the setter ?

@murtukov
Copy link
Member

murtukov commented Nov 11, 2020

@mcg-web the same with the setter, he just removed the type hint. I mean how can it possibly break anything? Or you mean the user-defined classes, that extend the interface, could stop working?

@mcg-web
Copy link
Member

mcg-web commented Nov 11, 2020

This is a public interface. It scope doesn't stop to the bundle, so yes this interface was implemented by other users since it was introduced to help doing that. Changing a public method of a non internal class or interface is a BC.

@murtukov
Copy link
Member

murtukov commented Nov 11, 2020

@mcg-web Then we return back to the previous questions:

  1. Is it a bug? Because the documentation says:

The bundle is totally "promise ready"

yet it fails to return a promise

  1. If yes, then what is more important, fixing the bug or stay BC?

@mcg-web
Copy link
Member

mcg-web commented Nov 11, 2020

There is always a way of fixing this without breaking BC (some dedicated getter and setter to deal with promise?) , that what I say higher up. I'm not arguing about fixing this bug but the "how" does not follow the release policies of this bundle. We only allow break BC for security reasons. The contract is while updating a minors or a patch release the bundle should not break anything.

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