Fix the issues reported by phpcs and phpstan

Hi

After applied the patch i run the phpcs command it shows some errors

vendor/bin/phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig web/modules/contrib/session_limit-3361326/

FILE: /var/www/html/vbd9/web/modules/contrib/session_limit-3361326/src/Services/SessionLimit.php
-------------------------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AND 1 WARNING AFFECTING 4 LINES
-------------------------------------------------------------------------------------------------------------------
251 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
308 | ERROR | Protected method name "SessionLimit::_onSessionCollision__Ask" is not in lowerCamel format
324 | ERROR | Protected method name "SessionLimit::_onSessionCollision__PreventNew" is not in lowerCamel format
341 | ERROR | Protected method name "SessionLimit::_onSessionCollision__DropOldest" is not in lowerCamel format
-------------------------------------------------------------------------------------------------------------------

FILE: /var/www/html/vbd9/web/modules/contrib/session_limit-3361326/src/Form/SessionLimitForm.php
------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------
135 | ERROR | Doc comment is empty
------------------------------------------------------------------------------------------------

FILE: /var/www/html/vbd9/web/modules/contrib/session_limit-3361326/src/Form/SettingsForm.php
-----------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------
16 | WARNING | The class short comment should describe what the class does and not simply repeat the class name
-----------------------------------------------------------------------------------------------------------------

FILE: /var/www/html/vbd9/web/modules/contrib/session_limit-3361326/src/Event/SessionLimitCollisionEvent.php
-----------------------------------------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
-----------------------------------------------------------------------------------------------------------
8 | ERROR | Doc comment is empty
13 | ERROR | Missing short description in doc comment
18 | ERROR | Missing short description in doc comment
23 | ERROR | Missing short description in doc comment
28 | ERROR | Missing short description in doc comment
-----------------------------------------------------------------------------------------------------------

FILE: /var/www/html/vbd9/web/modules/contrib/session_limit-3361326/src/Event/SessionLimitDisconnectEvent.php
------------------------------------------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
------------------------------------------------------------------------------------------------------------
7 | ERROR | Doc comment is empty
12 | ERROR | Missing short description in doc comment
17 | ERROR | Missing short description in doc comment
22 | ERROR | Missing short description in doc comment
27 | ERROR | Missing short description in doc comment
------------------------------------------------------------------------------------------------------------

Time: 1.97 secs; Memory: 16MB

thanks,

Hi @dineshkumarbollu I have mentioned in the remaining task in the issue summary these need to fixed. that's why i kept the status to need work.

Thanks!

Hi

Someone cahnged the status to reviewed that's why i test the module.

Thanks

Created a patch along with the remaining issues. Please review.

   /**
+   * The bypass.
+   *
    * @var bool
    */

That description is repeating the property name.

   /**
+   * Implements shouldBypass()
+   *

Method descriptions must not start with Implements/Implement nor repeat the method name.

+/**
+ * Implements Session limit collision event.
+ */

Class descriptions must not start with Implements/Implement nor repeat the class name.

   /**
+   * The useractivesessions value.
+   *
    * @var int
    */
   protected $userActiveSessions;

The description is repeating the property name without saying what is its purpose.

* @param int $sessionId
- * @param AccountInterface $account
+ * The sessionId.
+ * @param \Drupal\Core\Session\AccountInterface $account
+ * The account.
* @param int $userActiveSessions
+ * The userActiveSessions.
* @param int $userMaxSessions
+ * The userMaxSessions.

All those descriptions are repeating the parameter name.

   /**
+   * Implements getSessionId().
+   *
    * @return int
+   *   Returns int.
    */
   public function getSessionId() {

The return value description must not start with Returns/Return nor repeat the return value type, which is already given in the previous line.

+  /**
+   * The current user.
+   *
+   * @var \Drupal\session_limit\Services\SessionLimit
+   */
+  protected $sessionLimit;

The description is not for that property.

+  /**
+   * Constructs a new SessionLimitForm object.
+   *
+   * @param \Drupal\Core\Session\AccountInterface $current_user
+   *   The current user instance.
+   * @param \Drupal\session_limit\Services\SessionLimit $session_limit
+   *   The current user instance.
+   * @param \Drupal\Core\Session\SessionManagerInterface $session_manager
+   *   The session manager service.
+   * @param \Drupal\Core\Datetime\DateFormatter $dateFormatter
+   *   The date formatter.
+   */

The class name is missing its namespace.
There is no need to say instance in the parameter description.
The same description is given for two different parameters.

Addressed #10. Updated patch.

+   * Constructs a new \Drupal\session_limit\Event\SessionLimitCollisionEvent
+   * constructor.
    *

A constructor constructs a new object, not a new constructor.
The description must be in a single line.

-   * @param AccountInterface $account
+   *   The sessionId.

It should be The session ID.

    * @param int $userActiveSessions
+   *   The user active session ID.

    * @param int $userMaxSessions
+   *   The maximum user session.

Given the parameter names, those descriptions are not correct.

+ * Get session ID.

A definite article is missing before session.
The verb needs to be declined on the third person singular.

Addressed #14. Updated patch.

Thanks for patch, here is feedback for patch #16.
Please remove all the t() methods from Test files. And check my comments below.

1. +++ b/tests/session_limit.test
   @@ -66,17 +73,17 @@ class SessionLimitBaseTestCase extends DrupalWebTestCase {
   +      $this->assertText($this->t('Log out'), $this->t('User is logged in under session @no.', ['@no' => $session_number]));
   

   no need for t() method here, use string and sprintf.

2. +++ b/tests/session_limit.test
   @@ -299,18 +306,18 @@ class SessionLimitBaseTestCase extends DrupalWebTestCase {
   -   * getInfo() returns properties that are displayed in the test selection form.
   +   * GetInfo() returns properties that are displayed in the test selection form.
   

   getInfo and GetInfo is 2 different methods. I would not make this change.

3. +++ b/tests/session_limit.test
   @@ -427,29 +435,24 @@ class SessionLimitLogoutTestCase extends SessionLimitBaseTestCase {
   +    // $roles = $this->sessionLimitMakeRoles($user);
   

   Why this line is going to be removed?

4. +++ b/tests/session_limit.test
   @@ -427,29 +435,24 @@ class SessionLimitLogoutTestCase extends SessionLimitBaseTestCase {
   +    // // @FIXME
   

   Let's add phpcs:disable

There are a lot of changes, let's switch to MR workflow, to simplify review.

Addressing #18.1 #18.2
Providing updated patch.

MR created. Please review.

Thanks, I provided review.

Mohd Sahzad → made their first commit to this issue’s fork.

Right, I think this is ready to be looked at. There were some additional changes that needed making. Would be keen on any feedback on the changes made (this included enabling gitlab-ci for the module and then making updates to pass phpstan tests)

PHPStan has only one issue and it depends on the other module.

PHPCS reports this: DI is required on this line:
https://git.drupalcode.org/issue/session_limit-3361326/-/blob/3361326-fi...

GitLab CI does not report any PHP_CodeSniffer warnings/errors.

Ready for review.

VladimirAus → credited Ambient.Impact → .

VladimirAus → credited fskreuz → .

Added credits for 🐛 Services\SessionLimit should type-hint dependencies using interfaces Needs review .

That's great!
Ready for release!

Committed and released! 🍻
Thanks everyone!

Automatically closed - issue fixed for 2 weeks with no activity.