Skip to content

[make:user] Legacy <= 5.3 & Doctrine Cleanup #1107

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

Merged
merged 1 commit into from
May 3, 2022

Conversation

jrushlow
Copy link
Collaborator

@jrushlow jrushlow commented Apr 25, 2022

@jrushlow jrushlow force-pushed the fix/doctrine-legacy branch 2 times, most recently from 3d38390 to ddc814e Compare April 25, 2022 11:13
@@ -28,7 +28,7 @@
"require-dev": {
"composer/semver": "^3.0",
"doctrine/doctrine-bundle": "^2.4",
"doctrine/orm": "^2.3",
"doctrine/orm": "^2.10.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

potentially controversial.. need to double check dependency dependencies to ensure this is possible... Reasoning being the bump to 10 / conflict w/ <2.9 is primarily related to the new exception classes introduced in 2.10 that will become interfaces in 3.0..

Copy link
Member

Choose a reason for hiding this comment

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

My initial thought is: it's ok. ORM 2.10 was released Oct 3rd, 2021. Maker now requires Symfony 5.4, which was released at the end of November 2021. So, if you have 5.4, you've been upgrading already. And bumping to 2.10 is a minor release bump.

@jrushlow jrushlow force-pushed the fix/doctrine-legacy branch from 230942e to c095088 Compare April 25, 2022 11:39
@jrushlow jrushlow changed the title WIP - Doctrine Legacy Cleanup [make:user] Legacy <= 5.3 & Doctrine Cleanup Apr 25, 2022
@jrushlow jrushlow added Status: Needs Review Needs to be reviewed Minor Minor Enhancement Feature New Feature and removed Minor Minor Enhancement labels Apr 25, 2022
Comment on lines -191 to 169
if ('password_hashers' === $keyName && isset($newData['security']['encoders'])) {
// fallback to "encoders" if the user already defined encoder config
$this->updatePasswordHashers($userConfig, $userClass, 'encoders');

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Re for situations where a user still has security.encoders set - we could do any of:
A) Throw an exception e.g. "MakerBundle does not support password encoders.."
B) Remove the existing security.encoders and continue with setting security.password_hashers

The problem with the latter is we dont have a way to tell the user that we're removing the existing encoders.. I suppose we could start linking make:user's IO to SecurityConfigUpdater or something to make this "async" but that just feels like a headache w/ not alot of gain.

If we just throw an exception, what action do we tell the user to take to correct the problem?

Copy link
Member

Choose a reason for hiding this comment

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

If encoders is set, let's throw a clear exception that explains to change to password_hashers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds like a plan

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Very complex, nice work! Minor comments

Comment on lines -191 to 169
if ('password_hashers' === $keyName && isset($newData['security']['encoders'])) {
// fallback to "encoders" if the user already defined encoder config
$this->updatePasswordHashers($userConfig, $userClass, 'encoders');

Copy link
Member

Choose a reason for hiding this comment

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

If encoders is set, let's throw a clear exception that explains to change to password_hashers.

@jrushlow jrushlow force-pushed the fix/doctrine-legacy branch from bce7788 to 616806d Compare May 3, 2022 14:07
@jrushlow jrushlow added Status: Needs Work Additional work is needed Status: Needs Review Needs to be reviewed and removed Status: Needs Review Needs to be reviewed Status: Needs Work Additional work is needed labels May 3, 2022
@weaverryan weaverryan force-pushed the fix/doctrine-legacy branch from 004a131 to e0c5f2b Compare May 3, 2022 17:30
@weaverryan
Copy link
Member

HUGE work - thanks Jesse!

@weaverryan weaverryan merged commit a52f295 into symfony:main May 3, 2022
@jrushlow jrushlow deleted the fix/doctrine-legacy branch May 3, 2022 17:42
@jrushlow jrushlow mentioned this pull request May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make:user & password_hashers: skip password_hashers update if possible
2 participants