8000 Update SelectQuery return typehints to match updated ResultSetInterface by kolorafa · Pull Request #18925 · cakephp/cakephp · GitHub
[go: up one dir, main page]
Skip to content

Update SelectQuery return typehints to match updated ResultSetInterface #18925

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: 5.x
Choose a base branch
from
Open

Update SelectQuery return typehints to match updated ResultSetInterface #18925

wants to merge 1 commit into from
+4 −4

Conversation

kolorafa
Copy link
Contributor

In relation to changes to ResultSetInterface

Change to SelectQuery @return interfaces to include both required template variables

related conversation: #18867
change:
923620b
https://github.com/cakephp/cakephp/pull/18806/files#diff-ef7a1551c15fd8984eb0cf77bd248348055e104689d622609e671f1974e7c53e

@kolorafa kolorafa marked this pull request as ready for review September 24, 2025 15:15
@kolorafa
Copy link
Contributor Author

How to re-run those CI/CD checks?

As coding standard did fail to fetch phpstan binary 👀

@dereuromark
Copy link
Member

cc @rochamarcelo

@rochamarcelo
Copy link
Contributor

Looks good.

@rochamarcelo
Copy link
Contributor

@kolorafa you could use the existing TSubject (https://github.com/cakephp/cakephp/pull/18925/files#diff-1ab7f7c61167076737415b6cbabb97d5d6a0e70e5adec87dc763135f8a1117b1L47)

@kolorafa
Copy link
Contributor Author

@rochamarcelo You mean like changing
@return \Cake\Datasource\ResultSetInterface<array-key, mixed>
into
@return \Cake\Datasource\ResultSetInterface<array-key, TSubject>
?

@rochamarcelo
Copy link
Contributor

@kolorafa yes, also for \Cake\Datasource\EntityInterface|array

@markstory markstory added this to the 5.2.9 milestone Sep 27, 2025
@kolorafa
Copy link
Contributor Author
kolorafa commented Oct 6, 2025 •
edited
Loading

Sorry for the delay.

Thinking if I should change
\Cake\ORM\ResultSetFactory<\Cake\Datasource\EntityInterface|array>

into
\Cake\ORM\ResultSetFactory<TSubject>
or
\Cake\ORM\ResultSetFactory<TSubject|array>

the same for
`\Cake\Datasource\ResultSetInterface<array-key, \Cake\Datasource\EntityInterface|mixed>

into
\Cake\Datasource\ResultSetInterface<array-key, TSubject>
or
\Cake\Datasource\ResultSetInterface<array-key, TSubject|mixed>

because in theory ResultSetInterface could contain any type/structure if formatters created and returing god-knowns-what $result = $formatter($result, $this); in line 747

@kolorafa
Copy link
Contributor Author
kolorafa commented Oct 6, 2025 •
edited
Loading 
Error: Parameter #1 $entity of method Cake\ORM\Table<array>::delete() expects Cake\Datasource\EntityInterface, array|Cake\Datasource\EntityInterface given.
Error: Parameter #1 $entity of method Cake\ORM\Table<array>::delete() expects Cake\Datasource\EntityInterface, array|Cake\Datasource\EntityInterface given.
Error: Parameter #1 $entity of method Cake\ORM\Table<array>::delete() expects Cake\Datasource\EntityInterface, array|Cake\Datasource\EntityInterface given.
Error: Property Cake\ORM\Query\SelectQuery<TSubject of array|Cake\Datasource\EntityInterface>::$resultSetFactory (Cake\ORM\ResultSetFactory<TSubject of array|Cake\Datasource\EntityInterface>) does not accept Cake\ORM\ResultSetFactory<array|Cake\Datasource\EntityInterface>.

The issue is because we allow "array" as a TSubject

* @template TSubject of \Cake\Datasource\EntityInterface|array
introduced in 2023-01-03

places that would need change/assert to allow arrays as a return values:

cakephp/src/ORM/Association/HasMany.php

Lines 555 to 556 in 50d106c

foreach ($query->all() as $assoc) {
$ok = $ok && $target->delete($assoc, $options);

cakephp/src/ORM/Association/BelongsToMany.php

Lines 618 to 619 in 50d106c

foreach ($hasMany->find('all')->where($conditions)->all()->toList() as $related) {
$success = $table->delete($related, $options);

cakephp/src/ORM/Association/DependentDeleteHelper.php

Lines 56 to 57 in 50d106c

foreach ($association->find()->where($conditions)->all()->toList() as $related) {
$success = $table->delete($related, $options);

Question, should I remove allowing array as a returned type, or update the code to assert/check if the deleted value is actually EntityInterface and throw CakeException if it's not?

Or better revert/skip last changes and skip them for now, as it might be better to introduce this change not in patch release but minor release?

@rochamarcelo
Copy link
Contributor

Ignore TSubject for now since it is causing more issues. Maybe we can use it in the future.

@kolorafa kolorafa force-pushed the selectquery-all-typehing-fix branch from fbafc02 to 28ed3f0 Compare October 6, 2025 15:23
@kolorafa
Copy link
Contributor Author
kolorafa commented Oct 6, 2025 •
edited
Loading

Froce-pushed branch without the last commit.

I would also like those changes to be introduced, but I agree that it does generate issues that should be introduced in patch release.

So this PR only fixes the bad/missing second template for typehinting issue, and should be ready to merge.
Failing tests doesn't sound to be related to changes.

@kolorafa kolorafa force-pushed the selectquery-all-typehing-fix branch from 28ed3f0 to 9327269 Compare October 6, 2025 18:02
@kolorafa
Copy link
Contributor Author
kolorafa commented Oct 6, 2025

Rebased :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
5.2.9
Development

Successfully merging this pull request may close these issues.
4 participants
@kolorafa @dereuromark @rochamarcelo @markstory
0