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

fixed annotations of parameter $columns in Table #3392

Merged
merged 1 commit into from Dec 10, 2018

Conversation

LukasVitek
Copy link
Contributor

@LukasVitek LukasVitek commented Dec 6, 2018

Q A
Type bug
BC Break no

Fixes #3380.

Summary

Fixed annotations of parameter $columns in Table. Parameter $columns is array of mixed, nowhere in the code does not treated with him as with an array of array.

Before the fix the code, phpStan reported an error with this code:

$table = new Table('tableName');
$table->addUniqueIndex(['col1', 'col2']);

Parameter #1 $columnNames of method Doctrine\DBAL\Schema\Table::addUniqueIndex() expects array<array>, array<int, string> given.

@morozov
Copy link
Member

morozov commented Dec 6, 2018

@LukasVitek is this related to #3380? Could you double check if those mixed[][] $columns/$columnNames are actually string[] $columnNames, not mixed[]? See the details in the related ticket.

@morozov morozov added this to the 2.9.1 milestone Dec 6, 2018
@morozov morozov self-assigned this Dec 6, 2018
@LukasVitek
Copy link
Contributor Author

LukasVitek commented Dec 6, 2018

Actually, yes, issue #3380 is very related.
And yes, it is always assumed that $columns/$columnNames are type of string[],
except for one case here:

if (is_numeric($columnName) && is_string($indexColOptions)) {
where it is verified if the variable in the array is type of string.
Should I therefore type of annotations to remake to string[]?

@morozov
Copy link
Member

morozov commented Dec 6, 2018

Yes, please use string[] where makes sense and rename the variables from $columns to $columnNames to avoid further confusion.

koftikes
koftikes previously approved these changes Dec 7, 2018
lib/Doctrine/DBAL/Schema/Table.php Outdated Show resolved Hide resolved
@morozov
Copy link
Member

morozov commented Dec 8, 2018

@LukasVitek please squash and I’ll merge it.

@thewilkybarkid
Copy link

columnsAreIndexed() needs updating too.

@morozov
Copy link
Member

morozov commented Dec 10, 2018

@LukasVitek please update columnsAreIndexed() as well.

@morozov morozov dismissed Ocramius’s stale review December 10, 2018 23:31

The concern has been resolved.

@morozov morozov merged commit e4b18a4 into doctrine:master Dec 10, 2018
@morozov
Copy link
Member

morozov commented Dec 10, 2018

Thank you @LukasVitek and @thewilkybarkid.

morozov added a commit that referenced this pull request Dec 10, 2018
@leofeyer
Copy link
Contributor

leofeyer commented Dec 14, 2018

I'm afraid this change breaks indexes with a fixed length. We have an index named path(768), which now throws a There is no column with name 'path(768)' exception, because the following code has been removed in the Table::_createIndex() method:

        if (is_numeric($columnName) && is_string($indexColOptions)) {
            $columnName = $indexColOptions;
        }

@Ocramius
Copy link
Member

@leofeyer can you please report a new issue? Got a test case for it?

@leofeyer
Copy link
Contributor

@Ocramius Sure, here you go: #3409

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2.9 BC Break on Doctrine\DBAL\Schema\Table method signatures
6 participants