Skip to content

[make:reset-password] Add required packages to MakeResetPassword. #935

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

Closed

Conversation

bdaler
Copy link
Contributor

@bdaler bdaler commented Aug 6, 2021

As mentioned in issue: #911 add the packages symfony/form and symfony/validator as required.

@weaverryan
Copy link
Member

Thanks for this @bdaler!

Before I merge, one question is: why don't our tests catch this?

My guess is due to these lines: https://github.com/symfony/maker-bundle/blob/main/tests/Maker/MakeResetPasswordTest.php#L141 - at least some of these should not be here.

These "extra dependencies" should be used sparingly - usually only if a maker command has some optional dependency, but we want to install it in the test to make sure the code generates correctly WITH that optional dependency. Looking at the list in the test, I'm wondering if we should remove ALL of these - even symfony-bundle and twig should probably be moved to REAL dependencies in the maker itself - the command relies on security being installed and Twig. The same is true (I think) for doctrine, mailer, & doctrine (doctrine/annotations should be an optional dependency.... I think... in case you're using PHP 8 attributes, which is a totally different complication - so probably that one should be left alone for now).

@jrushlow can you verify that i'm correct? That we should probably remove all of the "extra dependencies" in the test (with the exception, perhaps, of doctrine/annotations) and move them to real dependencies of the maker?

@weaverryan
Copy link
Member

Update: I had missed #930 - I believe that has the correct behavior.

@weaverryan weaverryan closed this Aug 16, 2021
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.

2 participants