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

Implemented enum #434

Merged
merged 23 commits into from
Feb 1, 2024
Merged

Implemented enum #434

merged 23 commits into from
Feb 1, 2024

Conversation

uldisn
Copy link
Contributor

@uldisn uldisn commented May 24, 2020

Q A
Is bugfix? no
New feature? yes
Breaks BC? no
Tests pass? yes

Generate:

  • enum value constants
  • rules type in
  • opts
  • get enum field label (mybe rename )
  • is[EnumField][enumValue]
class Test extends \yii\db\ActiveRecord 
{
    const TYPE_PARTNER = 'PARTNER';
    const TYPE_PUBLIC = 'PUBLIC';
    const TYPE_INTERNAL = 'INTERNAL';
    const TYPE_OWNER = 'OWNER';

    public function rules()
    {
        return [
            [['type'], 'string'],
            [['position'], 'string', 'max' => 50],
            ['type', 'in', 'range' => [
                self::TYPE_PARTNER,
                self::TYPE_PUBLIC,
                self::TYPE_INTERNAL,
                self::TYPE_OWNER,
            ]
            ],
        ];
    }

   /**
     * column type ENUM value labels
     * @return array
     */
    public static function optsType()
    {
        return [
            self::TYPE_PARTNER => 'PARTNER',
            self::TYPE_PUBLIC => 'PUBLIC',
            self::TYPE_INTERNAL => 'INTERNAL',
            self::TYPE_OWNER => 'OWNER',
        ];
    }

    /**
    * @return string
    */
    public function displayType()
    {
        return self::optsType()[$this->type];
    }

    /**
     * @return bool
     */
    public function isTypePARTNER()
    {
        return $this->type === self::TYPE_PARTNER;
    }

    /**
     * @return bool
     */
    public function isTypePUBLIC()
    {
        return $this->type === self::TYPE_PUBLIC;
    }

    /**
     * @return bool
     */
    public function isTypeINTERNAL()
    {
        return $this->type === self::TYPE_INTERNAL;
    }

    /**
     * @return bool
     */
    public function isTypeOWNER()
    {
        return $this->type === self::TYPE_OWNER;
    }
}

@samdark samdark added the pr:request for unit tests Unit tests are needed. label May 25, 2020
@yii-bot
Copy link

yii-bot commented May 25, 2020

Thank you for putting effort in the improvement of the Yii framework.
We have reviewed your pull request.

In order for the framework and your solution to remain stable in the future, we have a unit test requirement in place. Therefore we can only accept your pull request if it is covered by unit tests.

Could you add these please?

Thanks!

P.S. If you have any questions about the creation of unit tests? Don't hesitate to ask for support. More information about unit tests

This is an automated comment, triggered by adding the label pr:request for unit tests.

@samdark samdark added the type:enhancement Enhancement label May 25, 2020
@uldisn
Copy link
Contributor Author

uldisn commented Dec 26, 2020

Unit test done

@samdark samdark added status:code review The pull request needs review. and removed pr:request for unit tests Unit tests are needed. labels Dec 26, 2020
2. To enum test added model->validation for rules testing
@bizley
Copy link
Member

bizley commented Dec 28, 2020

I've got few concerns:

  1. How this will work with MSSQL? IIRC ENUM dbType is recognized but enumValues are left empty.
  2. Overall this needs some tests with real DB engines.
  3. Are we cool with that much code being generated here?
  4. What do you think about having these optional/configured?

@uldisn
Copy link
Contributor Author

uldisn commented Dec 28, 2020

I've got few concerns:

  1. How this will work with MSSQL? IIRC ENUM dbType is recognized but enumValues are left empty.
    This is a problem for MS SQL users
  2. Overall this needs some tests with real DB engines.
    On MySql and MariaDB enum use more two years
  3. Are we cool with that much code being generated here?
    If use enum, is very useful. Code generate, if model has enums. Setting like $model->setTypePublic() also would also be good
  4. What do you think about having these optional/configured?
    Actually gii model form is so big. No need.

In giiant enum use long time: https://github.com/schmunk42/yii2-giiant/blob/master/src/generators/model/Generator.php#L295

@uldisn
Copy link
Contributor Author

uldisn commented Dec 29, 2020

  1. How this will work with MSSQL? IIRC ENUM dbType is recognized but enumValues are left empty.

Please provide me MSSQL schema dump like https://github.com/yiisoft/yii2-gii/pull/434/files#diff-eef4be0410fa93517728588e0c838dd2f9afb86ff7148c7f7d7636de99df3b00R486

@uldisn
Copy link
Contributor Author

uldisn commented Mar 30, 2021

@samdark! Actual PR still waiting. @bizley added additional strange requirements. It work.

@WinterSilence
Copy link
Contributor

WinterSilence commented Mar 31, 2021

@bizley

How this will work with MSSQL? IIRC ENUM dbType is recognized but enumValues are left empty.

mssql don't have enum/set types and they not required in sql standards

@bizley
Copy link
Member

bizley commented Mar 31, 2021

What do you mean by "strange"? These are legitimate concerns. You are just adding the feature but we will have to maintain it later on - I would like to at least be sure it works in most of the cases. And I'm not at the moment. I would like to see it tested with PostgreSQL since there is schema already in the tests added. And still no one said anything about the amount of additional code generated here (that is apart from you two guys, it's nice that you support each other like that).

src/generators/model/Generator.php Outdated Show resolved Hide resolved
src/generators/model/Generator.php Outdated Show resolved Hide resolved
src/generators/model/Generator.php Outdated Show resolved Hide resolved
src/generators/model/Generator.php Outdated Show resolved Hide resolved
src/generators/model/Generator.php Outdated Show resolved Hide resolved
tests/generators/ModelGeneratorTest.php Outdated Show resolved Hide resolved
tests/generators/ModelGeneratorTest.php Outdated Show resolved Hide resolved
tests/generators/ModelGeneratorTest.php Outdated Show resolved Hide resolved
tests/generators/ModelGeneratorTest.php Show resolved Hide resolved
tests/generators/ModelGeneratorTest.php Show resolved Hide resolved
src/generators/model/default/model.php Outdated Show resolved Hide resolved
tests/generators/ModelGeneratorTest.php Outdated Show resolved Hide resolved
2. enum rule optimised
3. added for enums to model methods set[EnumField[To[EnumValue]
@uldisn uldisn requested a review from bizley April 5, 2021 12:37
src/generators/model/Generator.php Outdated Show resolved Hide resolved
src/generators/model/Generator.php Outdated Show resolved Hide resolved
src/generators/model/Generator.php Outdated Show resolved Hide resolved
src/generators/model/default/model.php Outdated Show resolved Hide resolved
@uldisn uldisn requested a review from bizley April 6, 2021 09:25
@samdark samdark modified the milestones: 2.2.3, 2.2.4 Aug 6, 2021
@samdark samdark removed this from the 2.2.4 milestone Dec 30, 2021
@uldisn
Copy link
Contributor Author

uldisn commented Oct 31, 2023

I woke up

@samdark samdark added this to the 2.2.7 milestone Nov 2, 2023
@thiagotalma
Copy link
Contributor

Apparently he went back to sleep.

@samdark, what do you think about removing this PR from the 2.2.7 milestone and releasing the version?

@uldisn
Copy link
Contributor Author

uldisn commented Nov 28, 2023

Apparently he went back to sleep.

I define this initiative as illegal

@thiagotalma
Copy link
Contributor

I define this initiative as illegal

I prefer to believe that this was some kind of slang, as I don't understand English well.

But if you were bothered by my joke, I apologize.

My intention was just to clear the milestone so that a new version can be released.

@uldisn
Copy link
Contributor Author

uldisn commented Nov 28, 2023

I define this initiative as illegal

I prefer to believe that this was some kind of slang, as I don't understand English well.

But if you were bothered by my joke, I apologize.

My intention was just to clear the milestone so that a new version can be released.

Don't take it seriously. When should I wake up?

@samdark samdark removed this from the 2.2.7 milestone Dec 4, 2023
@samdark
Copy link
Member

samdark commented Jan 29, 2024

@uldisn time to wake up. Please add a line for CHANGELOG. Then I'll merge it.

@uldisn
Copy link
Contributor Author

uldisn commented Jan 29, 2024

@uldisn time to wake up. Please add a line for CHANGELOG. Then I'll merge it.

Done!
Good night!

@samdark samdark merged commit c7f7533 into yiisoft:master Feb 1, 2024
11 checks passed
@samdark
Copy link
Member

samdark commented Feb 1, 2024

Thank you!

@samdark samdark added this to the 2.2.7 milestone Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:code review The pull request needs review. type:enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants