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

Support BackedEnum attribute typecast behaviour. #20185

Merged
merged 3 commits into from
Dec 7, 2024

Conversation

briedis
Copy link

@briedis briedis commented Jun 4, 2024

Q A
Is bugfix?
New feature? ✔️
Breaks BC?

Based on the casting approach that Laravel has implemented.
Not sure about validation, I think that should be implemented separately.

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 64.84%. Comparing base (65e3369) to head (4b4eb9d).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
framework/behaviors/AttributeTypecastBehavior.php 0.00% 5 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #20185      +/-   ##
============================================
- Coverage     64.85%   64.84%   -0.01%     
- Complexity    11409    11411       +2     
============================================
  Files           431      431              
  Lines         37142    37146       +4     
============================================
  Hits          24087    24087              
- Misses        13055    13059       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@samdark samdark requested review from a team June 11, 2024 16:19
@briedis
Copy link
Author

briedis commented Nov 27, 2024

Any hope for this one?

Copy link
Member

@samdark samdark left a comment

Choose a reason for hiding this comment

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

Looks alright overall. Would you please add a line for CHANGELOG? Thanks.

@samdark samdark added this to the 2.0.52 milestone Dec 6, 2024
@mtangoo
Copy link
Contributor

mtangoo commented Dec 6, 2024

@briedis can you please add a line in the changelog? Seems last thing before merging!

@briedis
Copy link
Author

briedis commented Dec 6, 2024

I added the changelog entry.

What do you think about my second commit? Renaming enum cases so they match the suggested formatting (Pascal case) as in PHP's RFC. It may feel weird, but imho it is a better approach, as enums can't be confused with constants.

@mtangoo mtangoo requested review from samdark, bizley and Arhell December 6, 2024 17:36
@mtangoo
Copy link
Contributor

mtangoo commented Dec 6, 2024

I don't see an issue with that. Unless we have something implemented in the other way that can cause inconsistency. I can't think of any so I will leave for anyone who thinks otherwise before merging

@mtangoo
Copy link
Contributor

mtangoo commented Dec 7, 2024

@briedis thanks for your time and efforts!

@mtangoo mtangoo merged commit 49cfd3b into yiisoft:master Dec 7, 2024
37 of 87 checks passed
@@ -1538,13 +1538,13 @@ public function testBindValuesSupportsEnums()
$db = $this->getConnection();
$command = $db->createCommand();

$command->setSql('SELECT :p1')->bindValues([':p1' => enums\Status::ACTIVE]);
$command->setSql('SELECT :p1')->bindValues([':p1' => enums\Status::Active]);
$this->assertSame('ACTIVE', $command->params[':p1']);
Copy link
Contributor

Choose a reason for hiding this comment

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

The expected value should be renamed also.

Copy link
Contributor

Choose a reason for hiding this comment

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

@briedis can you prepare a PR to address this please. @Izumi-kun thanks for spotting and pointing this out

Copy link
Author

Choose a reason for hiding this comment

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

Sorry about that. Here's the PR: #20296

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to be sorry. It is OK.
Thanks for PR

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

Successfully merging this pull request may close these issues.

6 participants