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

Add type hints to codebase #1995

Merged
merged 28 commits into from Aug 3, 2021
Merged

Add type hints to codebase #1995

merged 28 commits into from Aug 3, 2021

Conversation

MasterOdin
Copy link
Member

This PR will add type hints to all possible places in the codebase. I debated on adding declare(strict_types=1); at the same time, but not sure if should be done at the same time, or separately. Putting this up in draft form to keep track of progress, and hopefully help ensure no one else spends time on doing this concurrently.

@dereuromark
Copy link
Member

I would hold off on the strict_types part, as this can create some more pitfalls where currently auto-casting works (to not break more than necessary).

{
$this->columns = $columns;
$this->columns = is_string($columns) ? [$columns] : $columns;
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 change makes this method equivalent to how setColumns works in the ForeignKey class.

Copy link
Member Author

Choose a reason for hiding this comment

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

Though the handling of the $columns value is very inconsistent across the codebase. A number of the AlterInstruction functions within the database adapters accepts a string $column property while some accept a Column $column, and then elsewhere we have string|string[] $columns or just string[] $columns. A follow-up PR would perhaps be good to work on making this more consistent throughout.

src/Phinx/Db/Table/Table.php Show resolved Hide resolved
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
src/Phinx/Config/Config.php Show resolved Hide resolved
src/Phinx/Db/Action/AddColumn.php Outdated Show resolved Hide resolved
src/Phinx/Db/Adapter/AdapterInterface.php Outdated Show resolved Hide resolved
Copy link
Member

@othercorey othercorey left a comment

Choose a reason for hiding this comment

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

Reviewed conventions. Left a couple open questions about valid types but just from a logic point of view.

I skipped several adapters that were likely exact copies of the base adapter.

@dereuromark
Copy link
Member

Shall we continue/finish this for the next branch?

MasterOdin and others added 4 commits July 28, 2021 13:56
Co-authored-by: othercorey <corey.taylor.fl@gmail.com>
Co-authored-by: othercorey <corey.taylor.fl@gmail.com>
Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
@MasterOdin
Copy link
Member Author

@othercorey requested changes have been made.

@dereuromark the failing phpstan test is due to a bug in phpstan: phpstan/phpstan#5369. Could maybe pin the used version to an older one that doesn't have this bug?

@MasterOdin MasterOdin marked this pull request as ready for review July 28, 2021 14:27
@dereuromark
Copy link
Member

@othercorey What do you think about the float thingie? Should we keep as is, or do we want to internally work with integers here (as one intuitively would)?

@othercorey
Copy link
Member

Looks like limit is the same as length in cakephp. We cast length to int so doesn't seem like it would hurt to do that here. I don't think there are any fractional length types.

Is it possible to cast the limit when reflecting?

@MasterOdin
Copy link
Member Author

For the MySQL adapter, the following values are problematic if declaring a int limit:

public const TEXT_LONG = 4294967295;

public const BLOB_LONG = 4294967295;

public const INT_REGULAR = 4294967295;
public const INT_BIG = 18446744073709551615;

These values all exceed the value of INT_MAX on 32-bit systems and INT_BIG exceeds INT_MAX on 64-bit systems. While these values are set to match the max size of the respective column types, given that these values exist purely as a shorthand to get say bigint(20) when doing ->column('foo', 'integer', ['limit' => MysqlAdapter::INT_BIG], could just arbitrarily cap them to be values less than INT_MAX, and then can declare the limit clause as an integer again.

This would be BC breaking, but in a way that I'm not sure anyone would notice. The specifics here are that:

  • The values of the above constants would be different
  • Assuming the new value of TEXT_LONG is now 2147483647 instead of 4294967295, then values between 2147483647 and 4294967295 would be considered TEXT_LONG instead of TEXT_MEDIUM (this would apply to the other types in similar way)

However, again, given that these are supposed to be "magic constants" as it were and not to directly depend on the underlying value, should be fine to change?

@othercorey
Copy link
Member

These values all exceed the value of INT_MAX on 32-bit systems and INT_BIG exceeds INT_MAX on 64-bit systems. While these values are set to match the max size of the respective column types, given that these values exist purely as a shorthand to get say bigint(20) when doing ->column('foo', 'integer', ['limit' => MysqlAdapter::INT_BIG], could just arbitrarily cap them to be values less than INT_MAX, and then can declare the limit clause as an integer again.

Looks like this is implemented differently than length in cakephp (which is just reflecting the mysql metadata). This should probably be implemented differently, but that's a separate issue.

I leave it up to you. I was just reviewing the type conventions and noticed the type changed in an odd way there.

Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>
@MasterOdin
Copy link
Member Author

@othercorey @dereuromark I followed through with what I talked about above, giving the MysqlAdapter constants values pinned at PHP_INT_MAX so that setLimit can be an integer again.

Looks like this is implemented differently than length in cakephp (which is just reflecting the mysql metadata). This should probably be implemented differently, but that's a separate issue.

That's the expected usage of limit (or its alias length). The constants just exist as a way of specifying a different type for integer, string, and blob base types on mysql. When actually fetching columns from the DB, it's expected that their actual limit / length will be an integer well below PHP_INT_MAX as expected.

@dereuromark dereuromark merged commit d457803 into cakephp:0.next Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants