- Status changed to Needs review
about 1 year ago 7:19am 26 October 2023 - last update
about 1 year ago Patch Failed to Apply - 🇨🇳China skyredwang Shanghai
Here is the first draft, no tests. I am reusing the /user/password endpoint, if the HTTP method is POST, then this implementation assume it is for resetting password, if the HTTP method is PATCH, then it is for changing password.
- last update
about 1 year ago 30,438 pass - last update
about 1 year ago Patch Failed to Apply - Status changed to Needs work
about 1 year ago 9:53am 26 October 2023 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
-
+++ b/core/modules/user/src/Controller/UserAuthenticationController.php @@ -260,15 +262,52 @@ public function resetPassword(Request $request) { + if (!$account->isAuthenticated()) { + throw new BadRequestHttpException('Authentication is required to change your password.'); + }
Once the patch request is a separate route you can make this a route requirement.
-
+++ b/core/modules/user/src/Controller/UserAuthenticationController.php @@ -260,15 +262,52 @@ public function resetPassword(Request $request) { + // Set existing password if set in the form state.
No form state here.
-
+++ b/core/modules/user/user.routing.yml @@ -132,8 +132,8 @@ user.pass: user.pass.http: path: '/user/password' defaults: - _controller: \Drupal\user\Controller\UserAuthenticationController::resetPassword - methods: [POST] + _controller: \Drupal\user\Controller\UserAuthenticationController::changeOrResetPassword + methods: [POST, PATCH] requirements: _access: 'TRUE' _format: 'json'
Let's add a different route for just patch. And let it have it's own controller. I don;t think we shoudl be renaming resetPassword here. And I think having a separate changePassword method is okay.
We can refactor the common parts of both methods into private functions on the controller.
-
- 🇨🇳China skyredwang Shanghai
The reason I didn't want to use a seperate controller is because the existing resetPassword uses
/user/password
./user/password
is generic, if I use a new path like/user/change_password
for changing password, then we have two end points for the consumer. One generic, one specific, that's confusing to me. What do you think?Attached improved the code comment per suggestion 2
- last update
about 1 year ago Patch Failed to Apply - last update
about 1 year ago Patch Failed to Apply - 🇺🇸United States bradjones1 Digital Nomad Life
Re-queued the patch since it's against 11.x but DrupalCI was running it against 10.1.
Also just want to say I love that this is using HTTP verb semantics to determine the operation to be performed!
- Status changed to Needs review
about 1 year ago 1:23am 27 October 2023 - last update
about 1 year ago Patch Failed to Apply - 🇨🇳China skyredwang Shanghai
I spoke with @alexpott on Slack, and understood that I can keep the same endpoint but defining a new route based on the new HTTP method.
I pushed the change to the issue fork 11.x
- @skyredwang opened merge request.
- Status changed to Needs work
about 1 year ago 1:47pm 27 October 2023 - 🇺🇸United States smustgrave
Hiding patches as work is in a MR with correct branch.
But appears to have some failures.
- Status changed to Needs review
about 1 year ago 7:16am 31 October 2023 - 🇨🇳China skyredwang Shanghai
The lastest version is functioning. It supports two use cases:
- Change password knowing the existing password
- Change password without knowing the existing password, but have the timestamp and token from forget password URLRemaining work to do:
- Tests
- Some of the code are copied from /core/modules/user/src/AccountForm.php and /opt/drupal/web/core/modules/user/src/Controller/UserController.php , better to re-organize them, but need suggestions, as I am not familiar with core. - Status changed to Needs work
about 1 year ago 5:36pm 7 November 2023 - 🇺🇸United States smustgrave
Passed tests, just needs it's own coverage as mentioned.