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

Incompatibility between getArrayResult() new type hint and use of indexBy() #8503

Closed
benjamintoussaint opened this issue Feb 22, 2021 · 9 comments · Fixed by #8547
Closed
Milestone

Comments

@benjamintoussaint
Copy link

Cs fixes1 #8475 brought this change of AbstractQuery which I find incompatible with the use of indexBy():
f833222#diff-65b30cb7319fd94159051fe212c87565a4ffd15a796fa0db4983c477076b88c8R798

@return array
@return array<int,mixed>

The new type hint is incompatible with a result indexed with a string property:

$qb = $userRepository
    ->createQueryBuilder('u')
    ->indexBy('u', 'u.firstName')
    ->getQuery()
    ->getArrayResult()
;

I noticed the same issue with the same change for getScalarResult().

I have thought of no alternative but to revert the change.

What do you think about this?

@stlrnz
Copy link

stlrnz commented Mar 2, 2021

I can confirm this issue.
In my case, i use groupBy() in combination with getArrayResult() and expect the array index to be string.

The updated return annotation released with 2.8.2 conflicts with my own annotation and causes Psalm to report some (false) issues.

@beberlei beberlei added this to the 2.8.3 milestone Mar 15, 2021
@stof
Copy link
Member

stof commented Mar 16, 2021

@stlrnz groupBy won't put string keys in the array. It is about adding a GROUP BY in the generated SQL query, not about indexing the hydrated result.

The case reported by @benjamintoussaint about indexBy is valid though.

@stlrnz
Copy link

stlrnz commented Mar 16, 2021

Yeah, I already noticed this. Sorry for the confusion.

@greg0ire
Copy link
Member

I have thought of no alternative but to revert the change.

@benjamintoussaint or we could simply do this: mixed[]

@ovidals
Copy link

ovidals commented Apr 8, 2021

Is this a breaking change? so no semantic versioning?

@stof
Copy link
Member

stof commented Apr 8, 2021

@ovidals that was a mistake when making the phpdoc types more precise (combined with an API where the precise return type depends on the query being executed). that was not an intentional breaking change (and it has already been fixed)

@ovidals
Copy link

ovidals commented Apr 8, 2021

@stof I see in the 2.8.4 there is another BC. I say that because we upgraded today from 2.8.1 to 2.8.4 and the app crashed due to an yml (it was? can't remember) error.

@stof
Copy link
Member

stof commented Apr 8, 2021

if you have another problem, please open a new issue to report it

@ThomasLandauer
Copy link
Contributor

ThomasLandauer commented Oct 30, 2021

@stlrnz Did you get indexBy to work in combination with groupBy? I came to the conclusion that this is not possible and just wanted to suggest this as a new feature. Example:

$this->createQueryBuilder('a', 'a.month')
    ->select('SUM(a.amount)')
    ->groupBy('a.month')
    ->getQuery()
    ->getArrayResult()
;

Actual result:

array:4 [
  0 => array:1 []
  1 => array:1 []
  2 => array:1 []
  3 => array:1 []
]

Desired result:

array:4 [
  "Jan" => array:1 []
  "Feb" => array:1 []
  "March" => array:1 []
  "April" => array:1 []
]

EDIT: Just found out myself: It needs to be included in the select: ->select('a.month, SUM(a.amount)')

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants