Add/improve type declarations for PHP 8.1 #2218
Conversation
othercorey
force-pushed
the
cakephpcs-5
branch
from
9151ae8
to
fd45b76
Compare
| @@ -6034,7 +6036,7 @@ public function testMigrationWithDropColumnAndForeignKeyAndIndex() | |||
| $adapter->disconnect(); | |||
|
|
|||
| $this->manager->setConfig($config); | |||
| $this->manager->migrate('production', '20190928205056'); | |||
| $this->manager->migrate('production', 20190928205056); | |||
There was a problem hiding this comment.
Some tests were passing string as a version instead of an int. This appears to only affect tests as the migration loading code converts to int.
othercorey
force-pushed
the
cakephpcs-5
branch
from
fd45b76
to
29cad57
Compare
| @@ -40,7 +42,7 @@ public function __construct(Table $table, Column $column) | |||
| * @param array<string, mixed> $options The column options | |||
| * @return static | |||
| */ | |||
| public static function build(Table $table, string $columnName, $type = null, array $options = []) | |||
| public static function build(Table $table, string $columnName, string|Literal $type, array $options = []): static | |||
There was a problem hiding this comment.
This appears to default to null only for a few tests which were omitting a type. This doesn't seem to be valid as the phinx code checks for either Literal or string. Updated tests to specify a type.
othercorey
force-pushed
the
cakephpcs-5
branch
from
29cad57
to
036fde2
Compare
| @@ -97,7 +103,7 @@ public function testConnectionWithSocketConnection() | |||
| } | |||
|
|
|||
| $options = ['unix_socket' => getenv('MYSQL_UNIX_SOCKET')] + MYSQL_DB_CONFIG; | |||
| $adapter = new MysqlAdapter(MYSQL_DB_CONFIG, new ArrayInput([]), new NullOutput()); | |||
| $adapter = new MysqlAdapter($options, new ArrayInput([]), new NullOutput()); | |||
There was a problem hiding this comment.
Wasn't passing the $config with socket configured so not sure if anything is running this test.
othercorey
force-pushed
the
cakephpcs-5
branch
2 times, most recently
from
a80eee8
to
7114e18
Compare
othercorey
force-pushed
the
cakephpcs-5
branch
from
7114e18
to
b6646f1
Compare
| * @return array | ||
| */ | ||
| protected function getIndexColums(int $tableId, int $indexId): array | ||
| protected function getIndexColums(string $tableId, string $indexId): array |
There was a problem hiding this comment.
This is passed strings not ints and is internal to this adapter so changed the types.
There was a problem hiding this comment.
An unfortunate "quirk" of PDO in that it returns numerical columns as strings.
src/Phinx/Db/Action/ChangeColumn.php
Outdated
| @@ -55,7 +57,7 @@ public function __construct(Table $table, string $columnName, Column $column) | |||
| * @param array<string, mixed> $options Additional options for the column | |||
| * @return static | |||
| */ | |||
| public static function build(Table $table, string $columnName, $type = null, array $options = []) | |||
| public static function build(Table $table, string $columnName, string|Column|Literal $type, array $options = []): static | |||
There was a problem hiding this comment.
Looks like the docstring on this function was wrong, and similar to above, we should only accept string|Literal here as that's what setType accepts.
| * @return array | ||
| */ | ||
| protected function getIndexColums(int $tableId, int $indexId): array | ||
| protected function getIndexColums(string $tableId, string $indexId): array |
There was a problem hiding this comment.
An unfortunate "quirk" of PDO in that it returns numerical columns as strings.
MasterOdin
changed the title
| * @param array<string, mixed> $options Column Options | ||
| * @throws \InvalidArgumentException | ||
| * @return $this | ||
| */ | ||
| public function addColumn($columnName, $type = null, array $options = []) | ||
| public function addColumn(string|Column $columnName, string|Literal $type, array $options = []) |
There was a problem hiding this comment.
This looks like it was possibly an accident? When supplying a Column instance in the first argument, it should already hold the type, right?
Error: src/Table.php:67:47: PossiblyNullArgument: Argument 2 of Phinx\Db\Table::addColumn cannot be null, possibly null value provided (see https://psalm.dev/078)
https://github.com/cakephp/migrations/actions/runs/6269091941/job/17025092704?pr=628
There was a problem hiding this comment.
Migrations support for phinx 0.15 is here cakephp/migrations#638
|
@MasterOdin You can override the header settings for phinx from the cakephpcs defaults now if you want. I don't think there will be any large-scale changes after this. You can also move away from cakephp-cs. It was useful for validating the type declarations and type-hints, but you could pull out the specific rules you want into a custom config. |
cakephp-codesniffer 5 is meant to be paired with cakephp 5 code. One of the key changes is requiring native type declarations where possible.