RFC: deprecate AbstractPlatform::getName()

[greg0ire]

I should have asked when reviewing https://github.com/doctrine/orm/pull/8870/files#diff-3e426fe47b58d2d4b970b1b7a0cbf5999af950e4f84e48dce4bee5061b57ed31R657 (cc @derrabus ), but what good is a getName() method when you can use instanceof? Isn't it more fragile?

In the past I remember Symfony deprecating form names, and recommending avoidance of services IDs that are not FQCN, and IMO this has makes things more clear. Should we get rid of AbstractPlatform::getName() as well?

[derrabus]

I still like getName() and does it really hurt to keep it? We've just used it recently, see symfony/symfony#42011.

[derrabus]

If I have to support multiple versions of DBAL, I would need to check for multiple base classes in order to detect a specific database engine. For instance, on DBAL 2.13, there is PostgreSQLPlatform, but that class is gone in DBAL 3.

[greg0ire]

Oh so the possible values of getName is more stable than the hierarchy across DBAL versions? I didn't realize that… in that case yes, I think deprecating getName would be a bad idea. Still, there is a lot to dislike here, for instance when you use postgresql in client code, it's hard to understand where that magic string is defined. Also, unlike getName() in Symfony forms, the name is not unique, but shared, for instance PostgreSQL94Platform uses postgresql as a name. Maybe it should have been called family or something like that?

It might feel better to have a list of supported families defined as constants in an enum-like class looking like TrimMode.

<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Platforms;

final class PlatformFamily
{
    public const POSTGRESQL = 'postgresql';
    // …

    /**
     * @codeCoverageIgnore
     */
    private function __construct()
    {
    }
}

WDYT?

[derrabus]

I don't see a big problem with magic strings here, but sure, having an enum might be helpful.

[morozov]

Should we get rid of AbstractPlatform::getName() as well?

Yes, it's been for a long time on my mind, just didn't ever get to it. We got rid of most of other getName()s previously.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.