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

Add known type casting to AnnotationToAttributeRector #6217

Merged
merged 4 commits into from
Aug 6, 2024

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Aug 6, 2024

@TomasVotruba TomasVotruba changed the title tv annot to attribute type cast Add known type casting to AnnotationToAttributeRector Aug 6, 2024
@TomasVotruba TomasVotruba force-pushed the tv-annot-to-attribute-type-cast branch from 1258558 to 7c4469d Compare August 6, 2024 09:47
Comment on lines 16 to 27
/**
* @var array<string, string[]>
*/
private const INTEGER_KEYS_BY_CLASS = [
AnnotationClassName::LENGTH => ['min', 'max'],
];

Copy link
Member Author

Choose a reason for hiding this comment

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

@etshy I made a static list to cast known keys. What other keys do you think we should include here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I only had the case on min/max for Length constraints so I'm not sure right now.
Basically any integer available ? I guess they all could be written as string in annotation but type hinted as int as Attributes.

I'll take a look of the Constraints list after work hours and could make a list from that

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess they all could be written as string in annotation but type hinted as int as Attributes.

Good point. I'll see if we can do reflection on existing attribute constructor and infer the int type from those.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had a few minute before going back to work so here a non exhaustive list if you still need it

CIDR
netmaskMin
netmaskMax

PasswordStrength
minScore

NoSuspiciousCharacters
checks (there are constants in docs but it's still an int)
restrictionLevel (same)

Range
max
min

File
filenameMaxLength

Image
maxHeight
maxPixels
maxRatio (float)
maxWidth
minHeight
minPixels
minRatio (float)
minWidth

Count
divisibleBy
max
min

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome, thanks for checking. I'll use this list to test this rule in the wild, once deployed.

@TomasVotruba TomasVotruba force-pushed the tv-annot-to-attribute-type-cast branch 3 times, most recently from 9b8805c to 4cda518 Compare August 6, 2024 11:52
@TomasVotruba TomasVotruba force-pushed the tv-annot-to-attribute-type-cast branch from a688600 to f0c57bc Compare August 6, 2024 12:30
@TomasVotruba TomasVotruba enabled auto-merge (squash) August 6, 2024 12:30
@TomasVotruba TomasVotruba merged commit 147c961 into main Aug 6, 2024
34 checks passed
@TomasVotruba TomasVotruba deleted the tv-annot-to-attribute-type-cast branch August 6, 2024 12:31
@@ -39,6 +39,10 @@ public function removeNullTypeFromUnionType(UnionType $unionType): UnionType
$unionedTypesWithoutNullType[] = $type;
}

if ($unionedTypesWithoutNullType !== []) {
Copy link
Member

Choose a reason for hiding this comment

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

on else, union with empty types is impossible, I think this can be handled by count($unionedTypesWithoutNullType) === 1

Copy link
Member

Choose a reason for hiding this comment

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

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.

Cast argument when converting Annotations to Attributes
3 participants