Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
I noticed that an entity query using count()
returns a string instead of an int, which contradicts the return type of int|array
in \Drupal\Core\Entity\Query\Sql\Query.php
:
/**
* Executes the query and returns the result.
*
* @return int|array
* Returns the query result as entity IDs.
*/
protected function result() {
if ($this->count) {
return $this->sqlQuery->countQuery()->execute()->fetchField();
}
// Return a keyed array of results. The key is either the revision_id or
// the entity_id depending on whether the entity type supports revisions.
// The value is always the entity id.
return $this->sqlQuery->execute()->fetchAllKeyed();
}
Steps to reproduce
// Returns a string instead of an int.
$count = \Drupal::entityTypeManager()->getStorage('user')->getQuery()
->count()
->execute();
Proposed resolution
Cast the count result to an integer. This will be good in preparation for using return type hints in the future.
/**
* Executes the query and returns the result.
*
* @return int|array
* Returns the query result as entity IDs.
*/
protected function result() {
if ($this->count) {
return (int) $this->sqlQuery->countQuery()->execute()->fetchField();
}
// Return a keyed array of results. The key is either the revision_id or
// the entity_id depending on whether the entity type supports revisions.
// The value is always the entity id.
return $this->sqlQuery->execute()->fetchAllKeyed();
}
Remaining tasks
Find if a similar problem exists anywhere else in code?
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3320240
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
solideogloria CreditAttribution: solideogloria commentedComment #3
solideogloria CreditAttribution: solideogloria commentedChanges have been pushed to the issue fork.
Comment #5
tobiasbComment #6
alexpottThere should be somewhere in the entity query tests that we can change to an assertSame or remove a cast to prove that this is working as expected.
Comment #7
alexpottNeeds work for #6
Comment #8
solideogloria CreditAttribution: solideogloria commentedMaybe something could be added to SqlContentEntityStorageTest.php? But I don't know enough to do that.
Comment #9
alexpott\Drupal\KernelTests\Core\Entity\EntityQueryTest
change all the assertEquals to assertSame for the count queries.Comment #10
solideogloria CreditAttribution: solideogloria commentedComment #11
smustgrave CreditAttribution: smustgrave at Mobomo commentedCan you update the MR to point to 10.1 please.
Comment #12
solideogloria CreditAttribution: solideogloria commentedHow do I do that? I don't know how to switch which branch a MR is targeting.
Comment #13
smustgrave CreditAttribution: smustgrave at Mobomo commentedIf you're the author of the MR. On https://git.drupalcode.org/project/drupal/-/merge_requests/2940 you should see an edit button.
Then on the next screen where it says "From xyz into project/xyz" there should be a dropdown where you can select the target branch.
Comment #14
solideogloria CreditAttribution: solideogloria commentedOkay, I did that. But now the merge includes a lot of things that aren't related.
Comment #15
solideogloria CreditAttribution: solideogloria commentedIs it possible to keep only the committed changes, rather than merging 9.4 into 10.1 or whatever it's doing?
Comment #16
smustgrave CreditAttribution: smustgrave at Mobomo commentedDo you have a copy of the changes you actually did? Can try and fix this but want to make sure your fix/tests are captured somewhere.
Essentially the MR branch is so far behind it's showing 1000s of changes so it needs to be rebased.
Comment #17
smustgrave CreditAttribution: smustgrave at Mobomo commentedAnother option is just close this MR and open a new one for 10.1 with your changes on that.
Comment #18
solideogloria CreditAttribution: solideogloria commentedYeah, these commits:
Comment #19
smustgrave CreditAttribution: smustgrave at Mobomo commentedOkay since the MR is so far behind going to close this one and reopen a new one with those changes from those commits.
Know this was a pain, find it to be the one negative about using MRs vs patches but most likely this change would go into 10.1 now.
Comment #22
solideogloria CreditAttribution: solideogloria commentedComment #23
solideogloria CreditAttribution: solideogloria commentedLooks ready to merge now.
Comment #24
smustgrave CreditAttribution: smustgrave at Mobomo commentedChanges look good once green I’ll mark it
Thanks!
Comment #25
smustgrave CreditAttribution: smustgrave at Mobomo commentedChanges look good and believe the test changes cover this.
Comment #27
catchCommitted/pushed to 10.1.x, cherry-picked back to 9.5.x, thanks!
Comment #28
mfbI'd recommend a change record here, since this could cause tests to fail if calling assertSame()
Comment #29
tobiasbFirst minimal draft of a CR https://www.drupal.org/node/3324755.
Comment #30
longwaveExpanded the change record a bit and published it, thanks.