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

Make ProxyQueryInterface generic #7909

Merged
merged 2 commits into from
Aug 21, 2022
Merged

Make ProxyQueryInterface generic #7909

merged 2 commits into from
Aug 21, 2022

Conversation

VincentLanglet
Copy link
Member

Subject

This will be for instance useful for batchActions, in order to infer the result of the query.

Changelog

### Added
- Added generics to the ProxyQueryInterface

@VincentLanglet VincentLanglet force-pushed the proxyQueryGeneric branch 2 times, most recently from f8f434f to 5976a41 Compare August 19, 2022 15:54
@VincentLanglet
Copy link
Member Author

This would be implemented this way for DoctrineORM
sonata-project/SonataDoctrineORMAdminBundle#1687

@VincentLanglet VincentLanglet requested review from jordisala1991 and a team August 19, 2022 15:57
@VincentLanglet
Copy link
Member Author

If you have time to take a look @jordisala1991

@jordisala1991
Copy link
Member

Do we need to change something in the docs like add a phpstan-param to infer the type of execute or is infer automatically?

If is manual, I think it would be nice to add it to docs (in the same example we have now)

@VincentLanglet
Copy link
Member Author

Do we need to change something in the docs like add a phpstan-param to infer the type of execute or is infer automatically?

If is manual, I think it would be nice to add it to docs (in the same example we have now)

We never add some phpstan-param in the doc. Not everyone is using static analysis or even know about it. It might be confusing to add some.

We didn't add any doc about the AdminInterface generic or CrudController. It's just how generics works and people should refer to the phpstan doc instead IMHO.

When you're using Phpstan, on level 6 it will require to add the generic type. So you'll end up having to write

@phpstan-param ProxyQueryInterface<Foo>

by yourself.

@@ -18,6 +18,9 @@

final class BatchOtherController
{
/**
* @param ProxyQueryInterface<object> $aCustomNameForTheProxyQuery
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I had in mind, I think it would be nice to be in docs too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the same idea that when you're doing Collection<Foo> or AdminInterface<Foo>.

But there is no documentation in Doctrine, or Symfony or Sonata about generics. I don't think that the place for it.

@jordisala1991
Copy link
Member

Do we need to change something in the docs like add a phpstan-param to infer the type of execute or is infer automatically?

If is manual, I think it would be nice to add it to docs (in the same example we have now)

We never add some phpstan-param in the doc. Not everyone is using static analysis or even know about it. It might be confusing to add some.

We didn't add any doc about the AdminInterface generic or CrudController. It's just how generics works and people should refer to the phpstan doc instead IMHO.

When you're using Phpstan, on level 6 it will require to add the generic type. So you'll end up having to write


@phpstan-param ProxyQueryInterface<Foo>

by yourself.

There could be a few cases:

  1. I use phpstan and I like if packages provide me with the examples including the generic thing
  2. I dont use phpstan and probably will remove those lines when copying the example code (not a big deal)
  3. I dont even know what phpstan is, and I dont even care about phpdoc, so I copy the full example (it wont break his app)
  4. I dont copy code, I use it as an example, so I might get useful information about how the code works or how is suposed to be used.

In all cases I think is good to show how are you supose to use Sonata code if you want the best quality. IMO having it on docs, makes people that read docs see that Sonata is improving its quality by providing some nice DX.

I dont say that we add all generics at once in this Pr, but I dont see a reason to not add them, if you use phpstan you will be thankful that we provide complete examples, if you dont you will remove it or not but wont hurt you.

@VincentLanglet VincentLanglet merged commit 99378c7 into 4.x Aug 21, 2022
@VincentLanglet VincentLanglet deleted the proxyQueryGeneric branch August 21, 2022 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants