User::setExistingPassword() does not return $this for chaining

Thanks, @Jaypan! The fix is straight. No need to add a test, no CS violation.

Added relevant issue.

This is an interesting case for BC. Technically, returning something different is a BC break. However, since it only ever returned NULL before, there's no reason a caller would do anything with the return value. So, I think this is OK.

However, we should add a unit test for this. Thanks!

Hi @xjm,

I have updated the patch with the test cases, Please review the patch.

Hi @jaypan,

Thanks for your valuable feedback. I have made changes in the test cases as mentioned in #7. Please review.

Please ignore the previous patch, Please review the latest one in which #7 is addressed.

BTW, would be better to alter an existed test to demonstrate the usage of chaining with setExistingPassword(), however, no such alterable test found.

+++ b/core/modules/user/tests/src/Functional/UserPasswordResetTest.php
@@ -458,4 +458,17 @@ public function testResetImpersonation() {
+    $user = $user->setExistingPassword('existing_pass');
+    $this->assertInstanceOf(User::class, $user);

Prefer one-line $this->assertInstanceOf(User::class, $user->setExistingPassword('existing_pass')); but it really does not matter.

The test added in #9 is good and enough. So RTBC +1.

+++ b/core/modules/user/tests/src/Functional/UserPasswordResetTest.php
@@ -458,4 +458,17 @@ public function testResetImpersonation() {
 
+  /**
+   * Tests that the user entity is created.
+   */
+  public function testExistingPassword() {
+    /** @var \Drupal\user\Entity\User $user */
+    $user = User::create([
+      'name' => $this->randomMachineName(),
+    ]);
+    $user->save();
+    $user = $user->setExistingPassword('existing_pass');
+    $this->assertInstanceOf(User::class, $user);
+  }
+

This could be done in a new kernel test - doing it in a new method on a functional test means we have to do a full install of Drupal.

Moved the test to UserEntityTest instead. Removed the ::save() call because it's not needed, also renamed the test case to be more descriptive of what we're actually testing.

Looks good to me.

Committed and pushed 51e7a08352 to 10.1.x and 0763535604 to 10.0.x and 6764848b9a to 9.5.x. Thanks!
Committed 844acce and pushed to 9.4.x. Thanks!

This broke PHPStan testing on 10+, please revert

Reverted from 9.5.x, 10.0.x, and 10.1.x.

I think the 9.5.x patch was fine, but the D10 patches need to do something with the phpstan baseline or something. https://www.drupal.org/pift-ci-job/2422757

This is the error on D10+ branches:

 ------ --------------------------------------------------------------------- 
  Line   core/modules/user/src/Entity/User.php                                
 ------ --------------------------------------------------------------------- 
         Ignored error pattern #^Method                                       
         Drupal\\user\\Entity\\User\:\:setExistingPassword\(\) should return  
         \$this\(Drupal\\user\\Entity\\User\) but return statement is         
         missing\.$# in path                                                  
         /var/www/html/core/modules/user/src/Entity/User.php was not matched  
         in reported errors.                                                  
 ------ --------------------------------------------------------------------- 


 [ERROR] Found 1 error                                                          

That means that the pattern indicated, that can be found in phpstan-baseline.json in the core directory for the 10+ branches, should be removed from that file since the error is not occurring anymore.

@alexpott fixed this.