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

[DoctrineBridge] Add missing break #46713

Merged
merged 1 commit into from Jun 19, 2022
Merged

[DoctrineBridge] Add missing break #46713

merged 1 commit into from Jun 19, 2022

Conversation

Sajito
Copy link

@Sajito Sajito commented Jun 19, 2022

This PR only adds the "no break" comment as requested on my last PR:
This PR only adds the missing break as requested on my last PR: #46676 (comment)

I hope it's fine to not include the describing table. The missing break is on purpose, so here is no real code change. The missing break should not have caused any side effects.

I have not changed any tests here, because both cases are already tested by these two lines:

$this->assertEquals([new Type(Type::BUILTIN_TYPE_OBJECT, false, EnumString::class)], $this->createExtractor()->getTypes(DoctrineEnum::class, 'enumString', []));
$this->assertEquals([new Type(Type::BUILTIN_TYPE_OBJECT, false, EnumInt::class)], $this->createExtractor()->getTypes(DoctrineEnum::class, 'enumInt', []));

@carsonbot
Copy link

It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you.

Cheers!

Carsonbot

@@ -200,6 +200,7 @@ public function getTypes($class, $property, array $context = [])
return [new Type(Type::BUILTIN_TYPE_ARRAY, $nullable, null, true, new Type(Type::BUILTIN_TYPE_INT), $enumType ?? new Type(Type::BUILTIN_TYPE_STRING))];
}
case Type::BUILTIN_TYPE_INT:
// no break, because type int and string are handled the same way, if $enumType is set
Copy link
Member

Choose a reason for hiding this comment

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

I meant BEFORE this case, aka between line 201 and 202 :)

Copy link
Author

Choose a reason for hiding this comment

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

Oh yeah, kinda obvious, sorry for the misunderstanding :)

I've changed the commit and added the missing break.

@Sajito Sajito changed the title [DoctrineBridge] Add "no break" comment [DoctrineBridge] Add missing break Jun 19, 2022
@nicolas-grekas
Copy link
Member

Thank you @Sajito.

@nicolas-grekas nicolas-grekas merged commit d686e38 into symfony:4.4 Jun 19, 2022
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.

None yet

3 participants