Add mysql specific indexes with lengths

[bburnichon]

First step to fix #1597 & #2368

[mnapoli]

I think length (singular) might be a better name than lengths (plural). Funny I ran into that issue just today.

[deeky666]

This is weird and we should not do this. This API accepts an array of field definitions and not an index. I see why you are doing this but this is more of a workaround/hack rather than a good solution. Also having lengths option on the index as an array of lengths for all of the index columns isn't better either. What we are missing is an IndexColumn class that has options itself dedicated to the index column but I am not sure we should/can add this in 2.x.

[bburnichon]

I know the API currently accepts an array of field names. My first intention was to change this to only accept an Index parameter. I agree that an IndexColumn could be a solution but it seems heavier to implement this class.

I did not wanted to have a simple array of lengths in the API but I currently do not see another better way to avoid this while keeping BC.

[bburnichon]

@mnapoli I made lengths plural to be consistent with columns, flags and so on.

[Ocramius]

no need for else

[bburnichon]

Don't you want to check $fields is of proper type before continuing? If I remove the else part, developers could call method with improper data type. Or do you just prefer to have a separate if statement instead of elseif?

[Ocramius]

You'd just make an if instead of an elseif ;-)

[aimeos]

Would be great to get this feature into DBAL because it would simplify managing indexes on text columns very much!

[tmaroschik]

What is necessary to get this pull request merged? Fixing the conflicts only or is the general approach in question?

[morozov]

@tmaroschik please provide a functional test which would fail without the fix. Additionally, as far as I understand the design decision made in AbstractPlatform::getIndexFieldDeclarationListSQL(), the usage of array $fields should be deprecated in favor of Index $index.

[bburnichon]

@morozov Is functional test provided sufficient?

[morozov]

@bburnichon yes, the test looks good. As far as I understand, only MySQL (and probably MariaDB) support.

Would it make sense to move the test up to SchemaManagerFunctionalTestCase and explicitly disable in all classes except MySqlSchemaManagerTest? It looks so to me because it covers the new 'lengths' flag of the index definition which belongs to the DBAL itself, not just to the MySQL schema manager.

[morozov]

@bburnichon please squash commits and I'll merge it.

[bburnichon]

@morozov Squashed and all tests green! \o/

[morozov]

Thank you @bburnichon, merged.

[exptom]

This doesn't seem to work on primary key definitions. e.g. I have defined this:

<id name="publicKeyCredentialId" type="base64">
            <options>
                <option name="lengths">255</option>
            </options>
</id>

The lengths option is silently ignored.

[bburnichon]

@exptom I did not check real behavior but I think that it is not valid anyway. A primary key cannot be partial as it need to ensure all values are unique.

[exptom]

@bburnichon Yes, you are right, sorry I need to rethink my schema!

[exptom]

@bburnichon Sorry to bring this up again, but I have refactored my entity and I am now just trying to apply the index lengths option to a normal index, like this:

<field name="publicKeyCredentialId" type="base64" />
<indexes>
            <index name="publicKeyCredentialId" columns="publicKeyCredentialId">
                <options>
                    <option name="lengths">200</option>
                </options>
            </index>
</indexes>

However this still doesn't work because I get this error:

PHP Warning: array_shift() expects parameter 1 to be array, string given in /home/tom/Workspaces/PHP_Libs/zf-mvc-users/vendor/doctrine/dbal/lib/Doctrine/DBAL/Schema/Index.php on line 109

which is because a string "200" is passed in rather than an array containing 200. How would I specify the index lengths correctly in the xml config?

[bburnichon]

I am afraid that it is because this feature was not implemented to XmlMapping. This is out of the scope of this PR. Have a look at https://github.com/doctrine/orm/blob/8c259ea5cb632dbb57001b2262048ae7fa52b102/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php#L993

[gggeek]

@exptom @bburnichon I just tested creation of a PK(191) on a varchar(255) column and it does work (at least in a mysql 5.6 db configured so that creation of the PK on the full length of the same column fails).

So to have support allowing partial-lenght PKs on Mysql shall I open a separate ticket and PR ?

[bburnichon]

@gggeek Even though MySQL allows it does not mean it should. A primary key must ensure all values are different, a partial key leaves room to add duplicates. Anyway, This ticket was merged around 2 years ago, if you wish to add support for partial-length primary key, you shall open a new ticket.

[gggeek]

@bburnichon I don't disagree with you - this is possibly one of those features of MySQL that come from those dark times when it was developed according to the 'make it stick' philosophy, regardless of correctness of data and query results ;-)
However it is also a useful feature to work around the dead stupid limitation of maximum index length on varchar columns - especially useful when the db config is such that this limit is as low as 190 characters...
Given the fact that both MySQL and DBAL already allow the definition of a Unique index that checks only a part of the data - a blatant logical nonsense - it stands to reason that we could allow as well the same 'relaxed' constraint in the definition of a PK.
I have thus created issue #4405

[devnix]

@bburnichon Sorry to bring this up again, but I have refactored my entity and I am now just trying to apply the index lengths option to a normal index, like this:

<field name="publicKeyCredentialId" type="base64" />
<indexes>
            <index name="publicKeyCredentialId" columns="publicKeyCredentialId">
                <options>
                    <option name="lengths">200</option>
                </options>
            </index>
</indexes>

However this still doesn't work because I get this error:

PHP Warning: array_shift() expects parameter 1 to be array, string given in /home/tom/Workspaces/PHP_Libs/zf-mvc-users/vendor/doctrine/dbal/lib/Doctrine/DBAL/Schema/Index.php on line 109

which is because a string "200" is passed in rather than an array containing 200. How would I specify the index lengths correctly in the xml config?

I am afraid that it is because this feature was not implemented to XmlMapping. This is out of the scope of this PR. Have a look at https://github.com/doctrine/orm/blob/8c259ea5cb632dbb57001b2262048ae7fa52b102/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php#L993

Hi @exptom, @bburnichon, sorry for reviving this thread after so long. I guess this comment could be out of scope, but I would like to note that I've been able to use this in XML:

<index name="test" columns="foo,bar,baz">
    <options>
        <option name="lengths">
            <value>100</value>
            <value>100</value>
        </option>
    </options>
</index>

generates the following diff:

CREATE INDEX test ON eventos_rol (foo(100), bar(100), baz);

I've still not found how to generate this diff:

CREATE INDEX test ON eventos_rol (foo, bar(100), baz);

but I guess this is a big step.

I guess that this possibility is not documented in the schema:

[bburnichon]

Hello. I guess it expects a `null` value. So maybe it could work with a `<value/>` as first element of list. If it does not work, you will have to reorder the columns so that first ones have their length given. Ie: ``` <index name="test" columns="foo,bar,baz"> <options> <option name="lengths"> <value/> <value>100</value> </option> </options> </index> ``` Or ``` <index name="test" columns="bar,foo,baz"> <options> <option name="lengths"> <value>100</value> </option> </options> </index> ``` A third way is to give the actual length of the column. Driver should detect that a complete column is indexed. Hope this helps. Le ven. 21 janv. 2022 à 19:00, Dev_NIX ***@***.***> a écrit :

…

@bburnichon <https://github.com/bburnichon> Sorry to bring this up again, but I have refactored my entity and I am now just trying to apply the index lengths option to a normal index, like this: <field name="publicKeyCredentialId" type="base64" /> <indexes> <index name="publicKeyCredentialId" columns="publicKeyCredentialId"> <options> <option name="lengths">200</option> </options> </index> </indexes> However this still doesn't work because I get this error: PHP Warning: array_shift() expects parameter 1 to be array, string given in /home/tom/Workspaces/PHP_Libs/zf-mvc-users/vendor/doctrine/dbal/lib/Doctrine/DBAL/Schema/Index.php on line 109 which is because a string "200" is passed in rather than an array containing 200. How would I specify the index lengths correctly in the xml config? I am afraid that it is because this feature was not implemented to XmlMapping. This is out of the scope of this PR. Have a look at https://github.com/doctrine/orm/blob/8c259ea5cb632dbb57001b2262048ae7fa52b102/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php#L993 Hi @exptom <https://github.com/exptom>, @bburnichon <https://github.com/bburnichon>, sorry for reviving this thread after so long. I guess this comment could be out of scope, but I would like to note that I've been able to use this in XML: <index name="test" columns="foo,bar,baz"> <options> <option name="lengths"> <value>100</value> <value>100</value> </option> </options> </index> generates the following diff: CREATE INDEX test ON eventos_rol (foo(100), bar(100), baz); I've still not found how to generate this diff: CREATE INDEX test ON eventos_rol (foo, bar(100), baz); but I guess this is a big step. I guess that this possibility is not documented in the schema: [image: image] <https://user-images.githubusercontent.com/1777519/150576775-5d7edeb9-710f-49c7-8300-efbf9966d829.png> — Reply to this email directly, view it on GitHub <#2412 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASTBJR47T5Q6AAHZETDJD3UXGNKTANCNFSM4CGRKAAQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[devnix]

Hi @bburnichon, thank you for your fast answer!

In my case, all these options

<option name="lengths">
    <value></value>
    <value>100</value>
</option>

<option name="lengths">
    <value/>
    <value>100</value>
</option>

<option name="lengths">
    <value xsi:nil="true"/>
    <value>100</value>
</option>

are throwing me an Argument 1 passed to Doctrine\DBAL\Schema\Index::Doctrine\DBAL\Schema\{closure}() must be of the type int or null, string given exception, as I guess it's being casted to string in https://github.com/doctrine/orm/blob/8c259ea5cb632dbb57001b2262048ae7fa52b102/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php#L1002

I have doubts if there is a current mechanism here to achieve this. I'm not inverting the order of the params, as I'm trying to map an already existing legacy database, and I'm trying to null the schema diff. I've tested the third approach and it generates a diff too.

[bburnichon]

I guess you will have to update parseOptions recursion and detect attribute name like it is done for fixed and unsigned. Add a special case for lengths and add a special evaluation method (evaluateIntegerOrNull in the same way evaluateBoolean).