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

restore comment hint (useful for enumtype declaration) #6444

Closed
wants to merge 24 commits into from

Conversation

meiyasan
Copy link

@meiyasan meiyasan commented Jun 17, 2024

Bug Report

Q A
Version 4.0.0

Summary

I add/register my custom EnumType at early stage to doctrine and then I am used to add proper comment hint for EnumType to allow for conversion from StringType to correct EnumType using DC2Type at EnumSubscriber::postGenerateSchema event stage; Doctrine was using CommentHint and it was useful. This has been removed from dbal in the commit 4134b86cb986f6fc68fe9ba7c8a7416debfa22bd for some reasons.

I cannot find any alternative, but rolling back a minimal code reintroducing comment hint to DC2Type detection for Doctrine to Type conversion. Not doing so creates a diff in the DB schema resulting in a systematic update of the database scheme every time I launch a doctrine:schema:update command, due to fictive difference in the type of the columns.

<?php

namespace App\DatabaseSubscriber;

use App\Database\Type\EnumType;
use Doctrine\DBAL\Schema\Column;
use Doctrine\ORM\Tools\Event\GenerateSchemaEventArgs;

class EnumSubscriber
{
    public function postGenerateSchema(GenerateSchemaEventArgs $eventArgs)
    {
        $columns = [];

        foreach ($eventArgs->getSchema()->getTables() as $table) {
            foreach ($table->getColumns() as $column) {
                if ($column->getType() instanceof EnumType) {
                    $columns[] = $column;
                }
            }
        }

        /** @var Column $column */
        foreach ($columns as $column) {
            
            $enum = $column->getType();
            $column->setComment(trim(sprintf('%s (%s)', $column->getComment(), implode(',', $enum::getPermittedValues()))));
        }
    }
}

Current behaviour

Current behavior, makes db schema update infinitely refreshing.

How to reproduce

Just create a EnumType following; and use postGenerateSchema subscriber.

Expected behaviour

I would expect to restore in AbstractSchemaManager.php:

    public function extractDoctrineTypeFromComment($comment, $currentType)

Then restore on every platform,

        $type = $this->platform->getDoctrineTypeMapping($dbType);
        $type                   = $this->extractDoctrineTypeFromComment($tableColumn['comment'], $type);

Unless this was suppressed on purpose, it was very useful feature to incorporate EnumType.
If an alternative is possible, please advice.

@meiyasan
Copy link
Author

I have restore the minimal code to make use of comment hint to detect EnumType based on (DC2Type:[xxx]) information as it was implemented before 4134b86cb986f6fc68fe9ba7c8a7416debfa22bd.
It allows to fix this issue implementing EnumType using EventListener::postGenerationSchema().

Please consider it, this was very useful feature and removed without much information to fix it.

@berkut1
Copy link
Contributor

berkut1 commented Jun 17, 2024

But can't you solve your problem by creating custom types?
https://www.doctrine-project.org/projects/doctrine-dbal/en/4.0/reference/types.html#custom-mapping-types

I use EnumType extensively in my project through custom types, and when testing with dbal4, I had no problems, and there shouldn't be any, because for Doctrine, it will still be int or string (depending on which EnumType you use).
The only thing you need to do when creating a type is to inherit from the parent Type and implement convertToDatabaseValue(), convertToPHPValue(), getSQLDeclaration(), and getBindingType().

@meiyasan
Copy link
Author

Adding the type to doctrine is not a problem, this is standard in the documentation.

The problem lies in the conversion back from doctrine to type. Previous behavior was to look for (DC2Type:XXX) in the comment hint.
At the moment, the field gets back into 'StringType' instead of the correct 'EnumType', so on request the db schema gets indefinitely upgraded as the diff is triggering update.

@berkut1
Copy link
Contributor

berkut1 commented Jun 18, 2024

Sorry if I misunderstood you, but the Doctrine looks at the type in the same way as it did with DC2Type, that is, you need to create and register the type you need (custom type).

UPD: If you are getting StringType, it means that you did not create your custom type or did not update it after switching to DBAL4.

@meiyasan
Copy link
Author

meiyasan commented Jun 18, 2024

Could you please provide an example?

I already highlighted the suppressed lines that are problematic. I don't think there are strong motivations to explain why comment hints have been removed. Looking at the commit there is actually not much explanation of that part and why comment hint has been removed. It seems to be a mixed modification wrapped up in another commit.

I cannot get this EnumType working, but I am sure there must be a way if you say so. I would be happy to give it a try. I added the EnumType to Doctrine configuration, I can use EnumType but this issue is taking place when going from Doctrine to Type. If you can suggest a doctrine event to listen to I would be happy to use it and to have this configured in time, but without this, the only solution I have is to use comment hint.

Comment hint is a quite reasonable feature and using hints in various context is a common practice including for queries. I don't see why we should remove a useful feature if it gives some flexibility to the implementation.

@meiyasan
Copy link
Author

#6257

@meiyasan
Copy link
Author

meiyasan commented Jun 18, 2024

All checks passed but codecov. (is that some key i should be providing ?)
Any maintainer please advice 🙏

@greg0ire
Copy link
Member

Regarding CodeCov, you have nothing to do, I reported it here: codecov/codecov-action#1487

@greg0ire
Copy link
Member

Regarding your issue, I think it might have been fixed with last week's 3.8.5 and 4.0.3 release. Have you tried upgrading, and disabling type comments?

@berkut1
Copy link
Contributor

berkut1 commented Jun 18, 2024

@greg0ire As I understand he uses DBAL4, not DBAL3.

@xkzl There is my custom type for int Enums.

enum Status: int
{
    case ACTIVE = 1;
    case SUSPENDED = 2;
    case ARCHIVED = 3;

    #[Pure]
    public function label(): string
    {
        return self::getLabel($this);
    }

    private function getLabel(self $value): string
    {
        return match ($value) {
            self::ACTIVE => 'STATUS_ACTIVE',
            self::SUSPENDED => 'STATUS_SUSPENDED',
            self::ARCHIVED => 'STATUS_ARCHIVED',
        };
    }

    public static function lists(): array
    {
        $cases = self::cases();
        $statuses = [];
        foreach ($cases as $status) {
            $statuses[$status->value] = $status->label();
        }
        return $statuses;
    }
}
final class StatusType extends Type
{
    final public const string NAME = 'user_user_status';

    #[\Override]
    public function convertToDatabaseValue(mixed $value, AbstractPlatform $platform): int
    {
        return $value instanceof Status ? $value->value : $value;
    }

    #[\Override]
    public function convertToPHPValue(mixed $value, AbstractPlatform $platform): ?Status
    {
        return !empty($value) ? Status::from($value) : null;
    }

    #[\Override]
    public function getSQLDeclaration(array $column, AbstractPlatform $platform): string
    {
        return $platform->getSmallIntTypeDeclarationSQL($column);
    }

    #[\Override]
    public function getBindingType(): int
    {
        return ParameterType::INTEGER;
    }

    public function getName(): string //I left this method for Symfony phpstorm plugin
    {
        return self::NAME;
    }
}

It works perfectly in DBAL4/ORM3, but I have to extend Type instead of SmallInt how it was in DBAL3, because they moved to strict types.

@meiyasan
Copy link
Author

meiyasan commented Jun 19, 2024

@berkut1 just to be on the same baseline here is the EnumType definition as proposed by Doctrine documentation https://www.doctrine-project.org/projects/doctrine-orm/en/3.2/cookbook/mysql-enums.html#solution-2-defining-a-type

I am interested to the registration type part and which doctrine event should be used, not really in your custom EnumType definition. How/where do you registered? I think this is the main point.

@greg0ire I just checked and in my latest checks I am using dbal 4.0.3, with orm 3.2.0. Could you advise how to disable type comment ?

Additionally, here is the latest reference to type comment I can find in the documentation and it is from dbal 3.8.5 https://www.doctrine-project.org/projects/doctrine-dbal/en/3.8/explanation/dc2type-comments.html
Having a page for 4.0 might be nice such that it would provide smooth example how to transition if not required anymore.

@berkut1
Copy link
Contributor

berkut1 commented Jun 19, 2024

How/where do you registered? I think this is the main point.

I register it through Symfony
https://symfony.com/doc/current/doctrine/dbal.html#registering-custom-mapping-types

Having a page for 4.0 might be nice such that it would provide smooth example how to transition if not required anymore.

There is no need transition, it just should work if you correctly created your custom types, especially getSQLDeclaration

@greg0ire
Copy link
Member

Types are always disabled in 4.0.x

@meiyasan
Copy link
Author

meiyasan commented Jun 19, 2024

How/where do you registered? I think this is the main point.

I register it through Symfony

https://symfony.com/doc/current/doctrine/dbal.html#registering-custom-mapping-types

Ok so you are not directly using the registration command mentioned in https://www.doctrine-project.org/projects/doctrine-dbal/en/4.0/reference/types.html#custom-mapping-types . Then, I think this is a different situation and it is done via doctrine configuration in your case. I think I better understand the differences between the two situations. My EnumType is part of a bundle. I don't think I can do it your way and I need à to listen to an event. I registered the type at the boot of my bundle using the initial command provided in the custom mapping type (basically Type::addType)

So according to my investigations, this issue is still here for 4.0.3 @greg0ire. So at this time, the only solution I can see is to use comment hints unless I missed something. Would you consider merging this PR @greg0ire, if it doesn't hurt current revisions/progress? If needed, I can additionally provide a test to make sure comment hints can get extracted so it will not be forgotten anymore.

@greg0ire
Copy link
Member

Would you consider merging this PR

DC2 comments are supposed to be completely gone in 4.0.x so… no?

@meiyasan
Copy link
Author

meiyasan commented Jun 19, 2024

What would be the alternative if we cannot add EnumType to bundles by registering manually through Type:addType on time for correct construction of columns...

@greg0ire
Copy link
Member

Use the enumType column attribute?

@meiyasan meiyasan closed this Jun 19, 2024
@meiyasan
Copy link
Author

meiyasan commented Jun 19, 2024

Thanks for trying.

@greg0ire
Copy link
Member

No worries. Also, it seems you can even avoid dealing with enumType if you use a recent enough version of PHP and the ORM:

As of version 2.11 Doctrine can also automatically map typed properties using a PHP 8.1 enum to set the right type and enumType.

@meiyasan
Copy link
Author

meiyasan commented Aug 7, 2024

I have yet to find a solution to the issue. Is there any plan to fix or address it?
So far, the only fix I've found is the provided file.

Additionally, I'd like to highlight the use case of SetType handling SET() in databases, on top of EnumType.

@meiyasan meiyasan reopened this Aug 7, 2024
@berkut1
Copy link
Contributor

berkut1 commented Aug 7, 2024

@xkzl can you check this theory? doctrine/migrations#1441 (comment)
Of course if you use ORM.

@meiyasan
Copy link
Author

meiyasan commented Aug 7, 2024

yes I can do that, maybe in few days only.

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm blocking the merge because I'm pretty sure we don't want to reintroduce those comments. I'm keeping the PR open so we can discuss alternatives.

@derrabus derrabus changed the base branch from 4.1.x to 4.2.x August 15, 2024 09:17
Lynnaut and others added 19 commits September 1, 2024 14:12
Update configuration.rst
…6513)

|      Q       |   A
|------------- | -----------
| Type         | bug
| Fixed issues | doctrine#6512 

#### Summary

If `false` (or anything that is not a string) is passed as `user` or
`password`, we run into a `TypeError` because we pass that value as-is
to the constructor of PDO. This started to happen after we enabled
strict types on our driver classes in 4.0.

On 3.9, `false` would implicitly be cast to an empty string which is
either desired or leads to more obscure connection errors. We could
restore the behavior of 3.9 by adding explicit type casts to the two
parameters. But since we don't document `false` as a valid value for
either parameter, my preference would indeed be raising an exception.
It is not referenced in any document
Headers are now supported with the native caption option.
It would seem this needs to be relative to the current document, which
is in the same directory.
Bumps [doctrine/.github](https://github.com/doctrine/.github) from 5.1.0
to 5.2.0.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a
href="https://github.com/doctrine/.github/releases">doctrine/.github's
releases</a>.</em></p>
<blockquote>
<h2>Deprecated Psalm job</h2>
<h2>Deprecated</h2>
<p>The static analysis workflow is deprecated in favor of a new workflow
that only runs PHPStan. Projects should:</p>
<ol>
<li>Migrate from <code>psalm-</code> prefixed annotations to
<code>phpstan-</code> prefixed annotations, or to unprefixed annotations
if they do not confuse PHPStorm.</li>
<li>Install <a
href="https://github.com/phpstan/phpstan-deprecation-rules">https://github.com/phpstan/phpstan-deprecation-rules</a></li>
<li>Remove Psalm</li>
<li>Migrate from <code>static-analysis.yml</code> to
<code>phpstan.yml</code></li>
</ol>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/doctrine/.github/commit/a233747c2982158037c82daf573a0af7d783448f"><code>a233747</code></a>
Merge pull request <a
href="https://redirect.github.com/doctrine/.github/issues/51">#51</a>
from greg0ire/split-sa-workflow</li>
<li><a
href="https://github.com/doctrine/.github/commit/c5b75c12851fa9fbb15273796f4f6096816213f3"><code>c5b75c1</code></a>
Split static analysis workflow</li>
<li><a
href="https://github.com/doctrine/.github/commit/86f67a236929d1e55401e34d852b0ef0588e52b3"><code>86f67a2</code></a>
Merge pull request <a
href="https://redirect.github.com/doctrine/.github/issues/52">#52</a>
from doctrine/maintenance</li>
<li><a
href="https://github.com/doctrine/.github/commit/0d3ee08e92bfe3dcf6c8748313105e747af6c740"><code>0d3ee08</code></a>
Avoid matrices unless needed</li>
<li><a
href="https://github.com/doctrine/.github/commit/04388b99374b1558abab39255eb4c4287ed133bd"><code>04388b9</code></a>
Remove comment about lib</li>
<li>See full diff in <a
href="https://github.com/doctrine/.github/compare/5.1.0...5.2.0">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=doctrine/.github&package-manager=github_actions&previous-version=5.1.0&new-version=5.2.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* 3.9.x:
  Bump doctrine/.github from 5.1.0 to 5.2.0 (doctrine#6534)
@derrabus
Copy link
Member

Closing in favor of #6536.

@derrabus derrabus closed this Oct 10, 2024
@meiyasan
Copy link
Author

@derrabus is your partial fix working with SetType ?

@derrabus
Copy link
Member

No.

@meiyasan
Copy link
Author

meiyasan commented Oct 10, 2024

Then for the record, and for anyone looking for a proper solution to this issue, here’s a code modifier that reintroduces the missing lines for the time being: https://packagist.org/packages/glitchr/doctrine-dc2type.

A reasonable fix, for the sake of people willing to use DBAL 4, while maintainers are struggling on the master branch...
you're welcome, sir.

@michnovka
Copy link

There is an open discussion to solve arbitrary custom types here doctrine/migrations#1441 and it does not require comment reintroduction.

@derrabus
Copy link
Member

A reasonable fix, for the sake of people willing to use DBAL 4

Sorry, no. That package is a horrible idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants