Fix inconsistencies with hasPrimaryKey

[DrewKolstad]

The array_diff() function was previously used incorrectly, which led to unexpected behavior. In the given scenario:

$primaryKey['columns'] = ['column1', 'column2', 'column3'];
$columns = ['column1', 'column2'];

$missingColumns = array_diff($columns, $primaryKey['columns']);

return empty($missingColumns);

The code would incorrectly return true because the existing primary key columns from $primaryKey['columns'] were not used as the source (or "from" argument) in the array_diff() function.

To address this issue, the proposed changes ensure that the existing primary key columns from $primaryKey['columns'] are correctly compared against the $columns method argument. This modification resolves the bug, making the comparison functionally correct.

This bug would not become apparent when removing one of the existing keys. It would only manifest when retaining the same columns and adding additional columns to the existing primary key.

[dereuromark]

Sounds like that part should be unit tested to avoid future regressions

[ndm2]

The array_diff() function was previously used incorrectly, which led to unexpected behavior. In the given scenario:

$primaryKey['columns'] = ['column1', 'column2'];
$columns = ['column1', 'column2', 'column3'];

$missingColumns = array_diff($columns, $primaryKey['columns']);

return empty($missingColumns);

The code would incorrectly return true [...]

It's very late over here, so maybe I'm just too sleepy, but that snippet there will not return true.

There is a problem with array_diff(), but it's a problem no matter the argument order, in one variant additional columns will cause a wrong result, in the other variant less columns will cause a wrong result.

That method should probably make a strict comparison, and only return true when both arrays have the same length and the same values... same order I'm not sure. Looks like it's possibly a similar problem as with the foreign keys.

[DrewKolstad]

The array_diff() function was previously used incorrectly, which led to unexpected behavior. In the given scenario:

$primaryKey['columns'] = ['column1', 'column2'];
$columns = ['column1', 'column2', 'column3'];

$missingColumns = array_diff($columns, $primaryKey['columns']);

return empty($missingColumns);

The code would incorrectly return true [...]

It's very late over here, so maybe I'm just too sleepy, but that snippet there will not return true.

There is a problem with array_diff(), but it's a problem no matter the argument order, in one variant additional columns will cause a wrong result, in the other variant less columns will cause a wrong result.

That method should probably make a strict comparison, and only return true when both arrays have the same length and the same values... same order I'm not sure. Looks like it's possibly a similar problem as with the foreign keys.

Doh! 🤦🏻‍♂️ You're absolutely right. Using array_diff() isn't a suitable solution in this case. I've updated the hasPrimaryKey() method to perform a comparison similar to what the hasForeignKey() method does.

[MasterOdin]

@DrewKolstad Sorry for the delay on getting this merged. Could you put together a unit test that demonstrates the original bug and how this fixes that? Failing that, do you have an example migration (or even just a sketch of one) that would demonstrate the original bug, and I can then put together the tests.

[MasterOdin]

Alright, here's a test case that demonstrates the fix:

    public function testHasPrimaryKeyMultipleColumns()
    {
        $table = new Table('table1', ['id' => false, 'primary_key' => ['column1', 'column2', 'column3']], $this->adapter);
        $table
            ->addColumn('column1', 'integer')
            ->addColumn('column2', 'integer')
            ->addColumn('column3', 'integer')
            ->save();

        $this->assertFalse($table->hasPrimaryKey(['column1', 'column2']));
    }

where on main this will fail and on this branch it'll pass.

[MasterOdin]

@DrewKolstad, @ndm2 I've made a commit that adds a test across all adapters that I think covers the bug and the fix, which passes here and fails on 0.x. Let me know if this is good, or if there's some additional test that should be made.

[ndm2]

Seeing that everything here uses mb_strtolower, all comparisons will be case-insensitive. This is different to how the foreign key comparisons are done, where Postgres and SQL Server are checked case-sensitive by default (because checking whether case-sensitivity applies is hard to maybe impossible).

To be honest, I'm not sure which way around is better, I guess there's pros and cons for bailing out early, just like there is for letting the DB trigger an error later down the road when fields actually do not exist because of case differences. In any case, there should probably be consistency in how these checks are done, foreign keys, primary keys, whatever keys.

[MasterOdin]

I think it's fine to be case sensitive here to err on the case of being more technically correct and avoid any potential issues where a user is relying on case sensitive to have similarly named columns sans case.

So I think just removing mb_strtolower from the postgres + sqlserver adapter code should be sufficient?

[ndm2]

That should probably be enough, yeah. And then some tests for case-sensitivity/insensitivity would probably be good, something along the lines of this:

https://github.com/cakephp/phinx/blob/8e678d165368ef3308dd448caa23f0084d7df1f4/tests/Phinx/Db/Adapter/MysqlAdapterTest.php#L1766

https://github.com/cakephp/phinx/blob/8e678d165368ef3308dd448caa23f0084d7df1f4/tests/Phinx/Db/Adapter/PostgresAdapterTest.php#L1710

[dereuromark]

So are we OK to continue here?