3.0.0 sql type hints by guidofaecke · Pull Request #373 · zendframework/zend-db

guidofaecke · 2019-03-16T23:31:37Z

Provide a narrative description of what you are trying to accomplish:

…s which do not add valuable information

…tain multiple spaces between the tag, type and description

… are impossible in current implementation

…mber)

…e the ? symbol present

Completed type hinting and added PHP 7.2 standards.

arueckauer · 2019-03-17T06:59:47Z

This PR provides changes described in #362

michalbundyra

Hi! I haven't review the whole PR (yet), but I caught some major issues with it which are repeated in the whole PR.

The PR has also some conflicts which needs to be resolved.

michalbundyra · 2019-03-17T09:18:27Z

.gitignore

@@ -1,3 +1,4 @@
+.idea/


We shouldn't have any IDE/OS specific files/directories here. Please revert and add it to your global .gitignore.

michalbundyra · 2019-03-17T09:19:50Z

src/Sql/AbstractSql.php

-    /**
-     * @var string
-     */
+    /** @var string[] */


this is invalid type, values are mixed, keys are sting

changed to mixed[]

michalbundyra · 2019-03-17T09:20:32Z

src/Sql/AbstractSql.php

+    public function getSqlString(?PlatformInterface $adapterPlatform = null) : string
    {
-        $adapterPlatform = ($adapterPlatform) ?: new DefaultAdapterPlatform;
+        $adapterPlatform = $adapterPlatform ?: new DefaultAdapterPlatform;


add parenthesis on class instantiation

parenthesis added back in

not these parentheses - on class instantiation so it should be: new DefaultAdapterPlatform()

Sorry about that - I guess I am starring at the code for too long 😆

michalbundyra · 2019-03-17T09:21:16Z

src/Sql/AbstractSql.php

-        DriverInterface $driver = null,
-        ParameterContainer $parameterContainer = null
-    ) {
+        PlatformInterface  $platform,


I am not sure about this parameters alignment, but if we are gonna go that way here we are missing on space

michalbundyra · 2019-03-17T09:22:11Z

src/Sql/AbstractSql.php

            // expression data)
            $values = $part[1];
-            $types = isset($part[2]) ? $part[2] : [];
+            $types  = isset($part[2]) ? $part[2] : [];


PHP 7.2 - use: $part[2] ?? []

michalbundyra · 2019-03-17T09:30:38Z

src/Sql/Ddl/Column/AbstractPrecisionColumn.php

-        $decimal = null,
-        $nullable = false,
-        $default = null,
+        string $name = '',


why we have default value here?

The original Column.php setName method is a bit confusing. Not sure which way to go here.

I can leave the null option in the constructor as it was and handle the null parameter in the setter to ensure '' (empty string)
or
force at least an empty string in the constructor.
Any good ideas here?

michalbundyra · 2019-03-17T09:31:36Z

src/Sql/Ddl/Column/Column.php

-     * @param mixed[]     $options
-     */
-    public function __construct($name = null, $nullable = false, $default = null, array $options = [])
+    public function __construct(string $name = '', bool $nullable = false, ?$default = null, array $options = [])


changed default value for $name, I think it should be noted

michalbundyra · 2019-03-17T09:31:58Z

src/Sql/Ddl/Column/Column.php

-     * @param mixed[]     $options
-     */
-    public function __construct($name = null, $nullable = false, $default = null, array $options = [])
+    public function __construct(string $name = '', bool $nullable = false, ?$default = null, array $options = [])


again: question mark before variable name (missing type?)

michalbundyra · 2019-03-17T09:33:02Z

src/Sql/Ddl/Column/Column.php

+     * @return self
     */
-    public function setDefault($default)
+    public function setDefault(?$default) : self


also here, question mark, please check all places

changed back, but open for discussion based on the PSR-12 draft

michalbundyra · 2019-03-17T09:34:24Z

src/Sql/Ddl/CreateTable.php

     * @param PlatformInterface $adapterPlatform
-     *
-     * @return string[][]|null
+     * @return string[][]|void


I think we are trying to avoid that, and do not mix void with the other type

# Conflicts: # src/Sql/Select.php

arueckauer · 2019-03-17T20:26:18Z

@webimpress Thanks for the feedback. There are quite a few cs breaks which have not been introduced by this PR. Instead of the laborious manual work of comparing and fixing everything, I suggest to either update zend-coding-standard to the latest 2.0.0-alpha.3 and use it or wait for the stable release. Btw a valid coding-standard is not in the scope of this issue.

guidofaecke · 2019-03-17T20:28:27Z

Ok folks, that's it for now.

weierophinney · 2019-12-31T22:43:04Z

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#9.

weierophinney · 2019-12-31T22:44:15Z

This repository has been moved to laminas/laminas-db. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

Squash all commits in your branch (git rebase -i origin/{branch})
Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
Run the laminas/laminas-migration tool on the code.
Clone laminas/laminas-db to another directory.
Copy the files from the second bullet point to the clone of laminas/laminas-db.
In your clone of laminas/laminas-db, commit the files, push to your fork, and open the new PR.
We will be providing tooling via laminas/laminas-migration soon to help automate the process.

guidofaecke and others added 30 commits March 14, 2019 06:45

Added some typehints

91d68c4

Added access modifiers for class constants

42e8770

Updated return type declarations

ae5c556

Use ::class instead

4a084fa

Use short list syntax

bda3610

Use origin function

fdc6130

Removed misusage of array_push()

4524104

Removed assignments where written value is same as default one

a55d7fd

Removed superfluous null assignments on class property initialization

18986fc

Write single-line comments with @var as one-liners

e4868fa

Updated file header comment, added strict_types declaration

b9ce140

Removed empty comment lines

6ebaf9d

Updated argument and return type declarations, removed method comment…

4ec4ee5

…s which do not add valuable information

Treat string class properties as not nullable where possible

788d1bc

Comment tags @param, @throws and @return should not be aligned or con…

f97e6f7

…tain multiple spaces between the tag, type and description

Revert short list syntax where incompatible

9565442

Ignore IDE files

9af735e

Ensure declarations are compatible with super, noted where type hints…

e057005

… are impossible in current implementation

Updated return type in DocBlock

2c385f7

Fixed case mismatch

c2ab9c3

Use self class referencing

0f8f3f4

Use single quotes

f4feef2

Remove unnecessary parentheses

6000d50

Remove unnecessary type casting, as the argument is of needed type

5866062

Place namespace definition on its own line

31c14f6

Use compact()

5f115d1

Converted elseif to if-branch (clearer intention, lower complexity nu…

a60c5c2

…mber)

Define one parameter per line in multi-line function declarations

402e21b

Removed unused imports

552623f

Fixed typo

a5d9333

arueckauer and others added 8 commits March 16, 2019 22:30

There must be exactly one space after a type cast

471da0c

Nullable and optional arguments, which are marked as = null, must hav…

636167e

…e the ? symbol present

Removed comments which do not add valuable information

73c4a59

Revert short list syntax where incompatible

5bb0ad8

Removed duplicate blank lines

aca883d

Merge pull request #1 from guidofaecke/3.0.0_SQL_typeHints

1c0ddf4

Completed type hinting and added PHP 7.2 standards.

Resolving merge conflict

f94abb7

Merge remote-tracking branch 'origin/3.0.0' into 3.0.0_SQL_typeHints

25d7ee3

michalbundyra suggested changes Mar 17, 2019

View reviewed changes

guidofaecke added 8 commits March 17, 2019 08:57

Resolving merge conflict

677d31a

Merge branch '3.0.0' into 3.0.0_SQL_typeHints

5901b66

# Conflicts: # src/Sql/Select.php

removed IDE/OS specifics

417ca86

fixed typehint

16dbb40

added parenthesis

da8b22b

changed self to $this

311ad6c

fixed column constructor

110f777

alignment and use function statements

65409db

comparator

ac29abe

guidofaecke added 2 commits March 17, 2019 13:41

parenthesis fixed

f748754

return fix

ba08bf4

weierophinney mentioned this pull request Dec 31, 2019

3.0.0 sql type hints laminas/laminas-db#9

Closed

17 tasks

Conversation

guidofaecke commented Mar 16, 2019

Uh oh!

arueckauer commented Mar 17, 2019

Uh oh!

michalbundyra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arueckauer commented Mar 17, 2019

Uh oh!

guidofaecke commented Mar 17, 2019

Uh oh!

weierophinney commented Dec 31, 2019

Uh oh!

weierophinney commented Dec 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants