bug #805 Enhancement: Fullfill interface and move comments to the method description (OskarStark)

weaverryan · weaverryan · commit 6536a581a1d2 · 2021-02-04T10:17:13.000-05:00
This PR was merged into the 1.0-dev branch. Discussion ---------- Enhancement: Fullfill interface and move comments to the method description Before this fix, static analyzers are complaining: ``` Return type (void) of method App\Entity\User::getSalt() should be compatible with return type (string|null) of method Symfony\Component\Security\Core\User\UserInterface::getSalt() ``` cc @chalasr Commits ------- e7957ba Enhancement: Fullfill interface
diff --git a/src/Security/UserClassBuilder.php b/src/Security/UserClassBuilder.php
@@ -165,13 +165,20 @@ private function addGetPassword(ClassSourceManipulator $manipulator, UserClassCo
             // add an empty method only
             $builder = $manipulator->createMethodBuilder(
                 'getPassword',
-                null,
-                false,
-                ['@see UserInterface']
+                'string',
+                true,
+                [
+                    'This method is not needed for apps that do not check user passwords.',
+                    '',
+                    '@see UserInterface',
+                ]
             );
+
             $builder->addStmt(
-                $manipulator->createMethodLevelCommentNode(
-                    'not needed for apps that do not check user passwords'
+                new Node\Stmt\Return_(
+                    new Node\Expr\ConstFetch(
+                        new Node\Name('null')
+                    )
                 )
             );
 
@@ -222,26 +229,38 @@ private function addGetPassword(ClassSourceManipulator $manipulator, UserClassCo
 
     private function addGetSalt(ClassSourceManipulator $manipulator, UserClassConfiguration $userClassConfig)
     {
-        // add getSalt(): always empty
+        if ($userClassConfig->hasPassword()) {
+            $methodDescription = [
+                'Returning a salt is only needed, if you are not using a modern',
+                'hashing algorithm (e.g. bcrypt or sodium) in your security.yaml.',
+            ];
+        } else {
+            $methodDescription = [
+                'This method is not needed for apps that do not check user passwords.',
+            ];
+        }
+
+        // add getSalt(): ?string - always returning null
         $builder = $manipulator->createMethodBuilder(
             'getSalt',
-            null,
-            false,
-            ['@see UserInterface']
+            'string',
+            true,
+            array_merge(
+                $methodDescription,
+                [
+                    '',
+                    '@see UserInterface',
+                ]
+            )
         );
-        if ($userClassConfig->hasPassword()) {
-            $builder->addStmt(
-                $manipulator->createMethodLevelCommentNode(
-                    'not needed when using the "bcrypt" algorithm in security.yaml'
-                )
-            );
-        } else {
-            $builder->addStmt(
-                $manipulator->createMethodLevelCommentNode(
-                    'not needed for apps that do not check user passwords'
+
+        $builder->addStmt(
+            new Node\Stmt\Return_(
+                new Node\Expr\ConstFetch(
+                    new Node\Name('null')
                 )
-            );
-        }
+            )
+        );
 
         $manipulator->addMethodBuilder($builder);
     }
diff --git a/tests/Security/fixtures/expected/UserEntityEmailWithPassword.php b/tests/Security/fixtures/expected/UserEntityEmailWithPassword.php
@@ -95,11 +95,14 @@ public function setPassword(string $password): self
     }
 
     /**
+     * Returning a salt is only needed, if you are not using a modern
+     * hashing algorithm (e.g. bcrypt or sodium) in your security.yaml.
+     *
      * @see UserInterface
      */
-    public function getSalt()
+    public function getSalt(): ?string
     {
-        // not needed when using the "bcrypt" algorithm in security.yaml
+        return null;
     }
 
     /**
diff --git a/tests/Security/fixtures/expected/UserEntityUser_nameWithPassword.php b/tests/Security/fixtures/expected/UserEntityUser_nameWithPassword.php
@@ -90,11 +90,14 @@ public function setPassword(string $password): self
     }
 
     /**
+     * Returning a salt is only needed, if you are not using a modern
+     * hashing algorithm (e.g. bcrypt or sodium) in your security.yaml.
+     *
      * @see UserInterface
      */
-    public function getSalt()
+    public function getSalt(): ?string
     {
-        // not needed when using the "bcrypt" algorithm in security.yaml
+        return null;
     }
 
     /**
diff --git a/tests/Security/fixtures/expected/UserEntityUsernameNoPassword.php b/tests/Security/fixtures/expected/UserEntityUsernameNoPassword.php
@@ -69,19 +69,23 @@ public function setRoles(array $roles): self
     }
 
     /**
+     * This method is not needed for apps that do not check user passwords.
+     *
      * @see UserInterface
      */
-    public function getPassword()
+    public function getPassword(): ?string
     {
-        // not needed for apps that do not check user passwords
+        return null;
     }
 
     /**
+     * This method is not needed for apps that do not check user passwords.
+     *
      * @see UserInterface
      */
-    public function getSalt()
+    public function getSalt(): ?string
     {
-        // not needed for apps that do not check user passwords
+        return null;
     }
 
     /**
diff --git a/tests/Security/fixtures/expected/UserEntityUsernameWithPassword.php b/tests/Security/fixtures/expected/UserEntityUsernameWithPassword.php
@@ -90,11 +90,14 @@ public function setPassword(string $password): self
     }
 
     /**
+     * Returning a salt is only needed, if you are not using a modern
+     * hashing algorithm (e.g. bcrypt or sodium) in your security.yaml.
+     *
      * @see UserInterface
      */
-    public function getSalt()
+    public function getSalt(): ?string
     {
-        // not needed when using the "bcrypt" algorithm in security.yaml
+        return null;
     }
 
     /**
diff --git a/tests/Security/fixtures/expected/UserModelEmailWithPassword.php b/tests/Security/fixtures/expected/UserModelEmailWithPassword.php
@@ -72,11 +72,14 @@ public function setPassword(string $password): self
     }
 
     /**
+     * Returning a salt is only needed, if you are not using a modern
+     * hashing algorithm (e.g. bcrypt or sodium) in your security.yaml.
+     *
      * @see UserInterface
      */
-    public function getSalt()
+    public function getSalt(): ?string
     {
-        // not needed when using the "bcrypt" algorithm in security.yaml
+        return null;
     }
 
     /**
diff --git a/tests/Security/fixtures/expected/UserModelUsernameNoPassword.php b/tests/Security/fixtures/expected/UserModelUsernameNoPassword.php
@@ -47,19 +47,23 @@ public function setRoles(array $roles): self
     }
 
     /**
+     * This method is not needed for apps that do not check user passwords.
+     *
      * @see UserInterface
      */
-    public function getPassword()
+    public function getPassword(): ?string
     {
-        // not needed for apps that do not check user passwords
+        return null;
     }
 
     /**
+     * This method is not needed for apps that do not check user passwords.
+     *
      * @see UserInterface
      */
-    public function getSalt()
+    public function getSalt(): ?string
     {
-        // not needed for apps that do not check user passwords
+        return null;
     }
 
     /**
diff --git a/tests/Security/fixtures/expected/UserModelUsernameWithPassword.php b/tests/Security/fixtures/expected/UserModelUsernameWithPassword.php
@@ -67,11 +67,14 @@ public function setPassword(string $password): self
     }
 
     /**
+     * Returning a salt is only needed, if you are not using a modern
+     * hashing algorithm (e.g. bcrypt or sodium) in your security.yaml.
+     *
      * @see UserInterface
      */
-    public function getSalt()
+    public function getSalt(): ?string
     {
-        // not needed when using the "bcrypt" algorithm in security.yaml
+        return null;
     }
 
     /**

Original file line number	Diff line number	Diff line change
`@@ -95,11 +95,14 @@ public function setPassword(string $password): self`
`95`	`95`	`}`
`96`	`96`
`97`	`97`	`/**`
	`98`	`+ * Returning a salt is only needed, if you are not using a modern`
	`99`	`+ * hashing algorithm (e.g. bcrypt or sodium) in your security.yaml.`
	`100`	`+ *`
`98`	`101`	`* @see UserInterface`
`99`	`102`	`*/`
`100`		`- public function getSalt()`
	`103`	`+ public function getSalt(): ?string`
`101`	`104`	`{`
`102`		`- // not needed when using the "bcrypt" algorithm in security.yaml`
	`105`	`+ return null;`
`103`	`106`	`}`
`104`	`107`
`105`	`108`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -90,11 +90,14 @@ public function setPassword(string $password): self`
`90`	`90`	`}`
`91`	`91`
`92`	`92`	`/**`
	`93`	`+ * Returning a salt is only needed, if you are not using a modern`
	`94`	`+ * hashing algorithm (e.g. bcrypt or sodium) in your security.yaml.`
	`95`	`+ *`
`93`	`96`	`* @see UserInterface`
`94`	`97`	`*/`
`95`		`- public function getSalt()`
	`98`	`+ public function getSalt(): ?string`
`96`	`99`	`{`
`97`		`- // not needed when using the "bcrypt" algorithm in security.yaml`
	`100`	`+ return null;`
`98`	`101`	`}`
`99`	`102`
`100`	`103`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -69,19 +69,23 @@ public function setRoles(array $roles): self`
`69`	`69`	`}`
`70`	`70`
`71`	`71`	`/**`
	`72`	`+ * This method is not needed for apps that do not check user passwords.`
	`73`	`+ *`
`72`	`74`	`* @see UserInterface`
`73`	`75`	`*/`
`74`		`- public function getPassword()`
	`76`	`+ public function getPassword(): ?string`
`75`	`77`	`{`
`76`		`- // not needed for apps that do not check user passwords`
	`78`	`+ return null;`
`77`	`79`	`}`
`78`	`80`
`79`	`81`	`/**`
	`82`	`+ * This method is not needed for apps that do not check user passwords.`
	`83`	`+ *`
`80`	`84`	`* @see UserInterface`
`81`	`85`	`*/`
`82`		`- public function getSalt()`
	`86`	`+ public function getSalt(): ?string`
`83`	`87`	`{`
`84`		`- // not needed for apps that do not check user passwords`
	`88`	`+ return null;`
`85`	`89`	`}`
`86`	`90`
`87`	`91`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -72,11 +72,14 @@ public function setPassword(string $password): self`
`72`	`72`	`}`
`73`	`73`
`74`	`74`	`/**`
	`75`	`+ * Returning a salt is only needed, if you are not using a modern`
	`76`	`+ * hashing algorithm (e.g. bcrypt or sodium) in your security.yaml.`
	`77`	`+ *`
`75`	`78`	`* @see UserInterface`
`76`	`79`	`*/`
`77`		`- public function getSalt()`
	`80`	`+ public function getSalt(): ?string`
`78`	`81`	`{`
`79`		`- // not needed when using the "bcrypt" algorithm in security.yaml`
	`82`	`+ return null;`
`80`	`83`	`}`
`81`	`84`
`82`	`85`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -47,19 +47,23 @@ public function setRoles(array $roles): self`
`47`	`47`	`}`
`48`	`48`
`49`	`49`	`/**`
	`50`	`+ * This method is not needed for apps that do not check user passwords.`
	`51`	`+ *`
`50`	`52`	`* @see UserInterface`
`51`	`53`	`*/`
`52`		`- public function getPassword()`
	`54`	`+ public function getPassword(): ?string`
`53`	`55`	`{`
`54`		`- // not needed for apps that do not check user passwords`
	`56`	`+ return null;`
`55`	`57`	`}`
`56`	`58`
`57`	`59`	`/**`
	`60`	`+ * This method is not needed for apps that do not check user passwords.`
	`61`	`+ *`
`58`	`62`	`* @see UserInterface`
`59`	`63`	`*/`
`60`		`- public function getSalt()`
	`64`	`+ public function getSalt(): ?string`
`61`	`65`	`{`
`62`		`- // not needed for apps that do not check user passwords`
	`66`	`+ return null;`
`63`	`67`	`}`
`64`	`68`
`65`	`69`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -67,11 +67,14 @@ public function setPassword(string $password): self`
`67`	`67`	`}`
`68`	`68`
`69`	`69`	`/**`
	`70`	`+ * Returning a salt is only needed, if you are not using a modern`
	`71`	`+ * hashing algorithm (e.g. bcrypt or sodium) in your security.yaml.`
	`72`	`+ *`
`70`	`73`	`* @see UserInterface`
`71`	`74`	`*/`
`72`		`- public function getSalt()`
	`75`	`+ public function getSalt(): ?string`
`73`	`76`	`{`
`74`		`- // not needed when using the "bcrypt" algorithm in security.yaml`
	`77`	`+ return null;`
`75`	`78`	`}`
`76`	`79`
`77`	`80`	`/**`