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

Narrow Model API #6261

Merged

Conversation

jordisala1991
Copy link
Member

@jordisala1991 jordisala1991 commented Aug 7, 2020

Subject

I am targeting this branch, because this is a BC break.

Changelog

### Changed
- Narrow the API for classes declared under the `Sonata\AdminBundle\Model\` namespace.

I have applied PHPStan level 8 when doing it and all of the errors related to the typehints are done.

src/Model/AuditReaderInterface.php Show resolved Hide resolved
src/Model/AuditReaderInterface.php Outdated Show resolved Hide resolved
src/Model/ModelManagerInterface.php Outdated Show resolved Hide resolved
@jordisala1991
Copy link
Member Author

I had to change a little more things:

I reduced the number of method on the test Datagrid.php (it had old methods that are already removed)
Removing them caused a bug on template where we didn't follow correctly this message (probably my bad):
https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Model/ModelManagerInterface.php#L266-L267

So I changed that too.

VincentLanglet
VincentLanglet previously approved these changes Aug 12, 2020
VincentLanglet
VincentLanglet previously approved these changes Aug 12, 2020
phansys
phansys previously approved these changes Aug 12, 2020
@jordisala1991 jordisala1991 force-pushed the major/add-type-hint-3 branch 2 times, most recently from 7336f04 to d86a02e Compare August 14, 2020 16:28
@jordisala1991
Copy link
Member Author

ping @sonata-project/contributors

VincentLanglet
VincentLanglet previously approved these changes Aug 22, 2020
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

class-string and array-key are not handled by Phpstorm.

I don't know if it's not better to write

@param string
@phpstan-param class-string

@jordisala1991
Copy link
Member Author

How are you dealing with this on dbal @greg0ire , vscode does not recognise it but does not complain either, and it becomes harder to spot those mistakes

@jordisala1991
Copy link
Member Author

Also when adding psalm do I have to write param, psalm-param and phpstan-param?

@greg0ire
Copy link
Contributor

greg0ire commented Aug 22, 2020

We are using prefixes (psalm- in dbal, phpstan- in annotations, which does not have Psalm yet). I think one of psalm-param or phpstan-param would be enough. Anyway, it's an issue with VSCode and PHPStorm, and I use none of those, so I don't know the details well enough.

@jordisala1991
Copy link
Member Author

The idea is to use it for array-key and for class-string? Also for using generics like: array<string, string> we should do like:

@param string[] $var
@phpstan-param array<string, string> $var

I guess phpstorm does not work for @template tag or does it work? Doing something like:

@template T of object

@param T $foo
@return T

Not sure if I should revert to basic phpdoc, or figure this out on this PR, to move on with the same idea on next PRs

@VincentLanglet
Copy link
Member

The idea is to use it for array-key and for class-string? Also for using generics like: array<string, string> we should do like:

@param string[] $var
@phpstan-param array<string, string> $var

Phpstorm understand array when you write array<string, string>. I think that's enough.
So on my projects I don't annoy myself and write.

@param array<string, string> $var

But for array-key and class-string, I prefer to use

@param string       $bar
@param string|int $foo

@phpstan-param class-string $bar
@phpstan-param array-key     $foo

In order to work with phpstorm.

I guess phpstorm does not work for @template tag or does it work? Doing something like:

@template T of object

@param T $foo
@return T

Indeed

@template T of object
 
@param object $foo
@phpstan-param T $foo

@return object
@phpstan-return T

seems better to me.

phansys
phansys previously approved these changes Aug 23, 2020
*/
public function getModelCollectionInstance($class);
public function getModelCollectionInstance(string $class): Collection;
Copy link
Member Author

Choose a reason for hiding this comment

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

This method on 3.x was deprecated, (will be removed once we merge 3.x with master, and we removed deprecated code again) but I wanted to show how this would have been (the relevant lines are the phpdoc, not this one). cc/ @VincentLanglet @phansys

Copy link
Member

Choose a reason for hiding this comment

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

I like this way.

@jordisala1991
Copy link
Member Author

We should be ready to go now. Please review @sonata-project/contributors

@jordisala1991 jordisala1991 merged commit 295d3af into sonata-project:master Aug 26, 2020
@jordisala1991 jordisala1991 deleted the major/add-type-hint-3 branch August 26, 2020 10:19
@jordisala1991
Copy link
Member Author

Lets have this one for now, Thank you for your reviews :)

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

Successfully merging this pull request may close these issues.

None yet

7 participants