Use @var instead of @param #3002
Conversation
|
There are ~10 issues like this in the code base. Could you please fix them all? |
| @@ -4,9 +4,24 @@ | |||
|
|
|||
| class ConnectionMock extends \Doctrine\DBAL\Connection | |||
| { | |||
| /** | |||
| * @var | |||
| */ | |||
| private $_fetchOneResult; | |||
There was a problem hiding this comment.
@morozov What should we use here?
There was a problem hiding this comment.
Likely mixed, try checking for the setter calls to discover its type.
There was a problem hiding this comment.
I guess is mixed. I can't find a single reference, even in Doctrine\DBAL\Connection 😞
There was a problem hiding this comment.
@Ocramius We are discussing the missing mixed here 😄
There was a problem hiding this comment.
If we remove it together with setFetchOneResult() and it doesn't break tests, I'd consider it the correct approach.
There was a problem hiding this comment.
Is up to you guys: mixed, or remove fetchColumn and setFetchOneResult methods?
There was a problem hiding this comment.
@carusogabriel please remove the unused methods if it doesn't affect existing tests.
| @@ -9,7 +9,14 @@ | |||
| */ | |||
| class ResultCacheTest extends \Doctrine\Tests\DbalFunctionalTestCase | |||
| { | |||
| /** | |||
| * @var Type | |||
There was a problem hiding this comment.
This seems wrong. This is int[][]|string[][]
There was a problem hiding this comment.
Almost all the array notations are reporting an error. I'm working on that 🚧
| @@ -4,9 +4,24 @@ | |||
|
|
|||
| class ConnectionMock extends \Doctrine\DBAL\Connection | |||
| { | |||
| /** | |||
| * @var | |||
There was a problem hiding this comment.
Just a comment to track the missing mixed
| private $expectedResult = array(array('test_int' => 100, 'test_string' => 'foo'), array('test_int' => 200, 'test_string' => 'bar'), array('test_int' => 300, 'test_string' => 'baz')); | ||
|
|
||
| /** | ||
| * @var \Doctrine\DBAL\Logging\DebugStack |
There was a problem hiding this comment.
Please move fully qualified names to the use section and use aliases.
There was a problem hiding this comment.
Shouldn't we add this to our CS as well?
There was a problem hiding this comment.
Looks like we do have the check for class names in the code but it doesn't work for comments. Inspecting:
<?php
$conn = new \Doctrine\DBAL\Connection();will produce:
------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------
3 | ERROR | [x] Class \Doctrine\DBAL\Connection should not be referenced via a fully qualified
| | name, but via a use statement.
------------------------------------------------------------------------------------------------
There was a problem hiding this comment.
@morozov Can you create brief repro code & report to Slevomat repo? It may've simply been overlooked since Slevomat guys afaik prefer FQCN in params/typehints. :)
There was a problem hiding this comment.
@Majkl578 please see slevomat/coding-standard#258.
There was a problem hiding this comment.
Ready to test: slevomat/coding-standard@c732809
| $_platform, | ||
| $_type; | ||
| /** | ||
| * @var MockPlatform |
There was a problem hiding this comment.
The MockPlatform doesn't define any new API which is not defined in AbstractPlatform. Could we declare this property as AbstractPlatform and consider the mock an implementation detail?
There was a problem hiding this comment.
@morozov What do you suggest?
There was a problem hiding this comment.
@carusogabriel @morozov is suggesting to simply document this as AbstractPlatform
There was a problem hiding this comment.
@Ocramius Done!
| @@ -4,9 +4,19 @@ | |||
|
|
|||
| class ConnectionMock extends \Doctrine\DBAL\Connection | |||
| { | |||
| private $_fetchOneResult; | |||
| /** | |||
| * @var DatabasePlatformMock | |||
There was a problem hiding this comment.
Unlike the previous platform mock, this type cannot be replaced with the abstract class (no action required).
| private $_platformMock; | ||
|
|
||
| /** | ||
| * @var SchemaManagerMock |
There was a problem hiding this comment.
It can be replaced with AbstractSchemaManager.
|
@carusogabriel let's fix whatever is identified now. We can fix newly identified issues consistently in another patch. |
|
🚢 |
As requested by @morozov in #2996