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

[make:*] add types for PHPStan #1542

Closed
wants to merge 2 commits into from
Closed

[make:*] add types for PHPStan #1542

wants to merge 2 commits into from

Conversation

seb-jean
Copy link
Contributor

@seb-jean seb-jean commented May 6, 2024

Hi,

I have add types for PHPStan for:

  • make:webhook
  • make:crud
  • make:message
  • make:registration-form
  • make:validator
  • make:reset-password

What do you think ?

@seb-jean seb-jean changed the title Add types for PHPStan [make:*] add types for PHPStan May 6, 2024
@seb-jean
Copy link
Contributor Author

seb-jean commented May 6, 2024

A commit is related to the issue: #1540

Copy link
Collaborator

@jrushlow jrushlow left a comment

Choose a reason for hiding this comment

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

Hey @seb-jean - awesome! A couple minor comments below and one request:

I'm working on v2 of SymfonyCasts ResetPasswordBundle && VerifyEmailBundle, I'm planning on introducing generics either in the last 1.x feature release or in the first 2.x release.

Tl;dr, can you split the changes to code that touches make:reset-password and make:registration from this PR into their own PR's here in maker bundle? e.g. [make:reset-password] phpstan types & [make:registration] a good title for the changelog Reasoning: it'll help me over in the symfonycasts repo's and I'll be able to tack on the changes made over there to your 2 new PR's.

Thank you in advance!

@@ -7,7 +7,7 @@

class <?= $class_name ?> extends ConstraintValidator
{
public function validate($value, Constraint $constraint)
public function validate(mixed $value, Constraint $constraint): void
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if this is possible (e.g. maker has enough knowledge about $value) but can we do string|array<int, mixed>|int|etc..... here instead of mixed? Thinking about phpstan level 9? where it complains if something is mixed or if an array does not define its shape.

I'm not suggesting we do all potential types as a union, but if we know $value could be say, either a string or a int - we should use string|int as the type instead of mixed.

else mixed is perfectly fine here 😉

Comment on lines 181 to 185
'getPassword',
'string',
'?string',
true,
[
'This method can be removed in Symfony 6.0 - is not needed for apps that do not check user passwords.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we even need to add this method anymore? I'm just looking at the comment that we add - maker only supports symfony 6.4+

If we do not need the method - lets leave this alone and remove the method in a separate PR.

@jrushlow jrushlow added Feature New Feature Status: Needs Work Additional work is needed labels May 6, 2024
@jrushlow jrushlow added this to the v1.60.0 milestone May 6, 2024
@seb-jean
Copy link
Contributor Author

seb-jean commented May 6, 2024

Closed in favor of other PRs I just created.

@seb-jean seb-jean closed this May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Status: Needs Work Additional work is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants