Skip to content

[TypeInfo] Fix resolve enum in string type resolver #59066

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 11, 2024
Merged

[TypeInfo] Fix resolve enum in string type resolver #59066

merged 1 commit into from
Dec 11, 2024
+9 −12

Conversation

DavidBadura
Copy link
Contributor

@DavidBadura DavidBadura commented Dec 2, 2024 •
edited by OskarStark
Loading

Q A
Branch? 7.1
Bug fix? yes
New feature? no
Deprecations? no
Issues ~
License MIT

Enum types are not handled correctly in the StringTypeResolver. They are always wrongly classified as object.

enum Status: string {
  case Pending = 'pending';
  case Closed = 'closed';
}

class Dto {
  /**
    * @var list<Status>
    */
  public array $statusList;
}

$reflection = new ReflectionProperty(Dto::class, 'statusList');
$type TypeResolver::create()->resolve($reflection);

$type instanceof BackedEnumType; // false -> should be true

@carsonbot carsonbot added this to the 7.3 milestone Dec 2, 2024
@carsonbot

This comment was marked as outdated.

Sign in to view
@DavidBadura DavidBadura force-pushed the fix-resolve-enum-in-string-type-resolver branch from 9e44043 to 5875006 Compare December 2, 2024 13:14
@DavidBadura DavidBadura changed the base branch from 7.3 to 7.2 December 2, 2024 13:14
@DavidBadura DavidBadura force-pushed the fix-resolve-enum-in-string-type-resolver branch from 5875006 to a9749dc Compare December 2, 2024 13:20
@DavidBadura DavidBadura changed the title fix resolve enum in string type resolver [TypeInfo] fix resolve enum in string type resolver Dec 2, 2024
@DavidBadura DavidBadura changed the title [TypeInfo] fix resolve enum in string type resolver [TypeInfo] Fix resolve enum in string type resolver Dec 2, 2024
@DanielBadura DanielBadura mentioned this pull request Dec 2, 2024
Merged
@DavidBadura DavidBadura force-pushed the fix-resolve-enum-in-string-type-resolver branch from a9749dc to 2fee2ff Compare December 3, 2024 14:35
@DavidBadura DavidBadura changed the base branch from 7.2 to 7.1 December 3, 2024 14:35
@DavidBadura DavidBadura closed this Dec 3, 2024
@DavidBadura DavidBadura reopened this Dec 3, 2024
@DavidBadura
Copy link
Contributor Author

windows test fail is not related

@DavidBadura
Copy link
Contributor Author

DavidBadura commented Dec 9, 2024 •
edited
Loading

@mtarld I hope it's okay to ping you, I have one question about my fix.

In ReflectionTypeResolver it is checked whether the enum is a BackedEnum and it is passed explicitly. But such a check also happens in the enum() function. I don't really understand why it is necessary to do this in the resolver as well.

see: https://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/TypeInfo/TypeResolver/ReflectionTypeResolver.php#L87

Is that necessary? I have transferred the relevant tests and they are running.

@OskarStark OskarStark modified the milestones: 7.3, 7.1 Dec 9, 2024
@mtarld
Copy link
Contributor

mtarld commented Dec 9, 2024

Hey @DavidBadura, thanks for your PR!

In ReflectionTypeResolver it is checked whether the enum is a BackedEnum and it is passed explicitly. But such a check also happens in the enum() function. I don't really understand why it is necessary to do this in the resolver as well.

Indeed, there is no reason to keep the BackedEnum check in ReflectionTypeResolver, I think you can remove it.

@carsonbot carsonbot changed the title [TypeInfo] Fix resolve enum in string type resolver Fix resolve enum in string type resolver Dec 9, 2024
@DavidBadura DavidBadura force-pushed the fix-resolve-enum-in-string-type-resolver branch from b48e3ce to e8bd29a Compare December 9, 2024 14:23
@chalasr chalasr force-pushed the fix-resolve-enum-in-string-type-resolver branch from e8bd29a to f4c4449 Compare December 11, 2024 12:11
@chalasr
Copy link
Member

chalasr commented Dec 11, 2024

Thank you @DavidBadura.

@chalasr chalasr merged commit 36bb990 into symfony:7.1 Dec 11, 2024
4 of 11 checks passed
@carsonbot carsonbot changed the title Fix resolve enum in string type resolver [TypeInfo] Fix resolve enum in string type resolver Dec 11, 2024
@DavidBadura DavidBadura deleted the fix-resolve-enum-in-string-type-resolver branch December 11, 2024 13:04
@xabbuh xabbuh mentioned this pull request Dec 12, 2024
Merged
xabbuh added a commit that referenced this pull request Dec 12, 2024
@xabbuh
minor #59194 [PropertyInfo] bump lowest required TypeInfo component v…
05ba52b 
…ersion (xabbuh)

This PR was merged into the 7.1 branch.

Discussion
----------

[PropertyInfo] bump lowest required TypeInfo component version

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

we need #59066 so that the test fixes from #59156 are also having an effect on the low deps job

Commits
-------

080f39e bump lowest required TypeInfo component version
This was referenced Dec 31, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof approved these changes

@mtarld mtarld mtarld approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed TypeInfo
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.
6 participants
@DavidBadura @carsonbot @mtarld @chalasr @stof @OskarStark