Fix the issues reported by phpcs

I applied patch #4, it applied cleanly, and all the errors was resolved (see attached images). It can be moved to RTBC.

Thanks

Charchil Khandelwal → made their first commit to this issue’s fork.

Thanks @samit.310@gmail.com, patch #4 applied cleanly, all errors and warnings are fixed.
Moving to RTBC.

Created MR for same.
Please review.

Thank You

Looks like a good catch overall by samit.310@gmail.com.

Reviewed the MR looks good to me.

After the Merge Request created by Charchil Khandelwal

Can be moved to RTBC.

Good work

    * @return \Symfony\Component\HttpFoundation\RedirectResponse
+   *   The Redirect Response.

Redirect and Response are misspelled.

-   * @param ExceptionEvent $event
+   * @param \Symfony\Component\HttpKernel\Event\ExceptionEvent $event
    *   Is given by the event dispatcher.

Since the documentation comment is changed, also the short description for that parameter needs to be changed.

           'title' => $this->t('Add redirect'),
-          'url' => Url::fromRoute('redirect.add', [], ['query' => ['source' => $path, 'language' => $result->langcode] + $destination]),
+          'url' => Url::fromRoute('redirect.add', [], [
+            'query' => [
+              'source' => $path,
+              'language' => $result->langcode,
+            ] + $destination,
+          ]),

Lines are allowed to be longer than 80 characters, if they are more readable. The changed code is not more readable.

-    $xpath = $this->xpath('//*[@id="edit-ignore-pages"]')[0]->getHtml();
+    // $xpath = $this->xpath('//*[@id="edit-ignore-pages"]')[0]->getHtml();
     // Check that the new page to ignore has been saved with leading slash.

Lines that must be removed are removed, not commented out.

   /**
-   * Passes if the row with the given parameters is NOT in the redirect_404 table.
+   * Passes if the row with the given parameters is NOT in redirect_404 table.

The existing short description is already correct. in the redirect_404 table is correct; in redirect_404 table is not correct.

+  /**
+   * Mock time.
+   *
+   * @var \Drupal\Component\Datetime\TimeInterface|\PHPUnit\Framework\MockObject\MockObject
+   */
+  protected $time;

It should be The mocked time.

+/**
+ * @file
+ * Module file for redirect_domain.
+ */

The usual short description for modules is Hook implementations for the [module name] module.

   /**
+   * The Redirect Checker.
+   *
    * @var \Drupal\redirect\RedirectChecker
    */
   protected $redirectChecker;

The short description does not say anything about the property purpose.

   /**
-   * Constructs a \Drupal\redirect\EventSubscriber\RedirectRequestSubscriber object.
+   * Constructs a DomainRedirectRequestSubscriber object.

The existing short description is already correct, since it includes the class namespace.

+/**
+ * Redirect generate form.
+ */
 function redirect_generate_form() {

The documentation form for a form builder function is different.

+/**
+ * Redirect generate form submit.
+ */

The short description for submission and validation handlers is different.

-  //$context['message'] = t('Deleted URL redirect @rid.', array('@rid' => end($rids)));
-
+  // $context['message'] = t('Deleted URL redirect @rid.',
+  // array('@rid' => end($rids)));

The existing code is more readable. It just misses a space after the comment delimiter.

+/**
+ * Redirect generate batch finish.
+ */
 function redirect_generate_batch_finished($success, $results, $operations) {

A batch callback is not described that way. Drupal coding standards have a section about callbacks.

+/**
+ * Redirect generate URL.
+ */
 function _redirect_generate_url($external = FALSE, $max_levels = 2) {

The documentation for the parameters or the return value is missing.

+/**
+ * Redirect generate querystring.
+ */
 function _redirect_generate_querystring() {

The short description is merely repeating the function name without underscores.

+/**
+ * Redirect status code.
+ */
 function redirect_status_code_options($code = NULL) {

The short description does not describe what the function does. Redirect status code. does not even make sense.
The parameter nor the return value are not described.

 /**
- * Implements hook_form_FORM_ID_alter() on behalf of locale.module.
+ * Implements hook_form_FORM_ID_alter() for locale.module.
  */
-function locale_form_redirect_edit_form_alter(array &$form, FormStateInterface $form_state) {
+function redirect_locale_form_redirect_edit_form_alter(array &$form, FormStateInterface $form_state) {

The function name is correct because, as the short description says, that hook is implemented on behalf of locale.module.
If I were to implement hook_entity_alter() in my module on behalf of the Node module, the function name would be node_entity_alter().

    * @return string
+   *   Source URL.
    */
   public function getSourceUrl() {

The short description is missing an article.

   /**
+   * The language Manager.
+   *
    * @var \Drupal\Core\Language\LanguageManagerInterface
    */
   protected $languageManager;

Manager is misspelled, since it is not the first word in a sentence.

   /**
+   * The entity type manager service.
+   *
    * @var \Drupal\Core\Entity\EntityTypeManagerInterface
    */
   protected $entityTypeManager;
 

There is no need to say it is a service.

   /**
+   * The Redirect Checker.
+   *
    * @var \Drupal\redirect\RedirectChecker
    */
   protected $checker;

Words are written capitalized in few cases, none of them include Redirect nor Checker.

   /**
+   * The path processor.
+   *
    * A path processor manager for resolving the system path.
    *
    * @var \Drupal\Core\PathProcessor\InboundPathProcessorInterface

The short description is sufficient.

   /**
-   * Constructs a \Drupal\redirect\EventSubscriber\RedirectRequestSubscriber object.
+   * The logger factory.
+   *
+   * @var \Drupal\Core\Logger\LoggerChannelFactoryInterface
+   */
+  protected $loggerFactory;
+
+  /**
+   * Constructs a RedirectRequestSubscriber object.

The existing short description is already correct, since it includes the class namespace.

 /**
- * A subscriber invalidating the 'rendered' cache tag when saving redirect settings.
+ * The RedirectSettingsCacheTag Class.
+ *
+ * A subscriber invalidating the 'rendered' cache tag when saving
+ * redirect settings.
  */
 class RedirectSettingsCacheTag implements EventSubscriberInterface {

The existing short description is already correct, since it does not repeat the class name.

+/**
+ * The RedirectDeleteForm class.
+ */
 class RedirectDeleteForm extends ContentEntityConfirmFormBase {

The short description for a class must not repeat the class name.

+  /**
+   * The entity type manager service.
+   *
+   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
+   */
+  protected $entityTypeManager;

It is not necessary to say it is a service, since a manager class is used for a service.

+  /**
+   * The module handler service.
+   *
+   * @var \Drupal\Core\Extension\ModuleHandlerInterface
+   */

It is not necessary to say it is a service.

+  /**
+   * Constructs a RedirectSettingsForm object.
+   *
+   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
+   *   The factory for configuration objects.
+   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
+   *   The module handler.
+   */
+  public function __construct(ConfigFactoryInterface $config_factory, ModuleHandlerInterface $module_handler) {

The class name must include its namespace.

+  /**
+   * The Redirect Repository.
+   *
+   * @var \Drupal\redirect\RedirectRepository
+   */
+  protected $redirectRepository;

Redirect and Repository are misspelled.

+   * @param \Drupal\Core\Routing\AccessAwareRouterInterface $router
+   *   A router class for Drupal with access check and upcasting.
+   * @param \Drupal\redirect\RedirectRepository $redirect_repository
+   *   A router class for Drupal with access check and upcasting.

The last short description is wrong. It is the exact same short description given for the other parameter.

-        // @todo - Hmm... exception driven logic. Find a better way how to
+        // @todo Hmm... exception driven logic. Find a better way how to
         //   determine if we have a valid path.

Find a better way how to verify the path is valid. is probably better.

-              ['%path' => $source_path, '@url-alias' => Url::fromRoute('entity.path_alias.add_form')->toString()]) . '</div>';
+              [
+                '%path' => $source_path,
+                '@url-alias' => Url::fromRoute('entity.path_alias.add_form')->toString(),
+              ]) . '</div>';

That code is not more readable.

+  /**
+   * Message string.
+   *
+   * @var string
+   */

The short description is missing an article. Furthermore, the short description does not need to say the property type.

   /**
+   * The contect.
+   *
    * @var \Symfony\Component\Validator\Context\ExecutionContextInterface
    */
   protected $context;

The short description contains a typo.

 /**
+ * Migrate process plugin.
+ *
  * @MigrateProcessPlugin(
  *   id = "d7_redirect_source_query"
  * )
@@ -28,7 +25,7 @@ class RedirectSourceQuery extends ProcessPluginBase {

The short description should be more specific. That same description has been used for two classes already.

+  /**
+   * Create RedirectChecker object.
+   */
   public function __construct(ConfigFactoryInterface $config, StateInterface $state, AccessManager $access_manager, AccountInterface $account, RouteProviderInterface $route_provider) {

The verb does not use the third-person singular.
The class namespace is missing.
The parameter descriptions are missing.

+/**
+ * The RedirectRepository class.
+ */
 class RedirectRepository {

The short description must not repeat the class name.

   /**
+   * Database connection.
+   *
    * @var \Drupal\Core\Database\Connection
    */
   protected $connection;

The short description is missing an article.

   /**
-   * Constructs a \Drupal\redirect\EventSubscriber\RedirectRequestSubscriber object.
+   * The request stack.
+   *
+   * @var \Symfony\Component\HttpFoundation\RequestStack
+   */
+  protected $requestStack;
+
+  /**
+   * Constructs RedirectRepository object.
    *
    * @param \Drupal\Core\Entity\EntityTypeManagerInterface $manager
    *   The entity type manager.
    * @param \Drupal\Core\Database\Connection $connection
    *   The database connection.
+   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
+   *   The configuration factory.
+   * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
+   *   The request stack.
    */
-  public function __construct(EntityTypeManagerInterface $manager, Connection $connection, ConfigFactoryInterface $config_factory) {
+  public function __construct(EntityTypeManagerInterface $manager, Connection $connection, ConfigFactoryInterface $config_factory, RequestStack $request_stack) {

The class namespace is missing from the constructor short description.

-   * @param $language
+   * @param string $language
    *   The language for which is the redirect.

Since the documentation comment is changed, also that short description must be corrected.

   /**
+   * The repository.
+   *
    * @var \Drupal\redirect\RedirectRepository
    */
   protected $repository;

That is a too vague short description, which could describe any repository.

   /**
+   * The storage.
+   *
    * @var \Drupal\Core\Entity\Sql\SqlContentEntityStorage
    */
-   protected $storage;
+  protected $storage;

The short description is too generic.

+  public function testParseUrl() {
+    // $test_cases = array(
+    // array(
+    // 'input' => array(
+    // 'b' => 'aa',
+    // 'c' => array('c2' => 'aa', 'c1' => 'aa'),
+    // 'a' => 'aa',
+    // ),
+    // 'expected' => array(
+    // 'a' => 'aa',
+    // 'b' => 'aa',
+    // 'c' => array('c1' => 'aa', 'c2' => 'aa'),
+    // ),
+    // ),
+    // );
+    // foreach ($test_cases as $index => $test_case) {
+    // $output = redirect_parse_url($test_case['input']);
+    // $this->assertIdentical($output, $test_case['expected']);
+    // }

The code indentation is wrong.
Since the Drupal coding standards say to use the short array syntax, that should be used even for commented out code.

-    // Maintenance mode is on, but user has access to view site in maintenance mode.
+    // Maintenance mode is on, but user has access to view site in
+    // maintenance mode.
     $accountWithMaintenanceModeAccess = $this->createMock('Drupal\Core\Session\AccountInterface');

Instead of reformatting the comment, that comment should be removed, as it does not say anything that is already clear from the code.

-//    $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state);
-//
-//    $route = $this->getMockBuilder('Symfony\Component\Routing\Route')
-//      ->disableOriginalConstructor()
-//      ->getMock();
-//    $route->expects($this->any())
-//      ->method('getOption')
-//      ->with('_admin_route')
-//      ->will($this->returnValue('system.admin_config_search'));
-//
-//    $request = $this->getRequestStub('index.php', 'GET',
-//      array(RouteObjectInterface::ROUTE_OBJECT => $route));
-//    $this->assertFalse($checker->canRedirect($request), 'Cannot redirect if we are requesting a admin path');
-//
-//    // We are at admin path with ignore_admin_path set to TRUE.
-//    $config['redirect.settings']['ignore_admin_path'] = TRUE;
-//    $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state);
-//
-//    $request = $this->getRequestStub('index.php', 'GET',
-//      array(RouteObjectInterface::ROUTE_OBJECT => $route));
-//    $this->assertTrue($checker->canRedirect($request), 'Can redirect a admin with ignore_admin_path set to TRUE');
+    // $checker = new RedirectChecker($this->getConfigFactoryStub($config),
+    // $state);
+    //
+    //    $route = $this->getMockBuilder('Symfony\Component\Routing\Route')
+    //    ->disableOriginalConstructor()
+    //    ->getMock();
+    //    $route->expects($this->any())
+    //    ->method('getOption')
+    //    ->with('_admin_route')
+    //    ->will($this->returnValue('system.admin_config_search'));
+    //
+    //    $request = $this->getRequestStub('index.php', 'GET',
+    //    array(RouteObjectInterface::ROUTE_OBJECT => $route));
+    //    $this->assertFalse($checker->canRedirect($request),
+    // 'Cannot redirect if we are requesting a admin path');
+    //
+    //    // We are at admin path with ignore_admin_path set to TRUE.
+    //    $config['redirect.settings']['ignore_admin_path'] = TRUE;
+    //    $checker = new RedirectChecker($this->getConfigFactoryStub($config),
+    //    $state);
+    //
+    //    $request = $this->getRequestStub('index.php', 'GET',
+    //    array(RouteObjectInterface::ROUTE_OBJECT => $route));
+    //    $this->assertTrue($checker->canRedirect($request),
+    //    'Can redirect aadmin with ignore_admin_path set to TRUE');

Formatting rules do not change whether the code is commented out. Those changes are wrong.

-   * @param $redirect
+   * @param string $redirect
    *   The redirect entity.

If it is a redirect entity, its type cannot be a string.

-   * @param $method
+   * @param string $method
    *   Method to mock - either load() or findMatchingRedirect().

The short description is missing an article.
I would rather use a comma instead of an hyphen.

-   * @param $url
-   *   Url to be returned from getRedirectUrl
+   * @param string $url
+   *   Url to be returned from getRedirectUrl.
    * @param int $status_code
    *   The redirect status code.

Url is misspelled.
The short description is missing an article.

-   * @param $path_info
-   * @param $query_string
+   * @param string $path_info
+   *   Path info.
+   * @param string $query_string
+   *   Query string.

The short descriptions are each missing an article.

   * @return \Drupal\language\ConfigurableLanguageManagerInterface|\PHPUnit\Framework\MockObject\MockObject
+   *   The $language_manager.

That does not describe the return value.

     return [
-      ['https://example.com/route-to-normalize', [], 'https://example.com/route-to-normalize', FALSE],
-      ['https://example.com/route-to-normalize', ['key' => 'value'], 'https://example.com/route-to-normalize?key=value', FALSE],
-      ['https://example.com/index.php/', ['q' => 'node/1'], 'https://example.com/?q=node%2F1', TRUE],
-      ['https://example.com/index.php/', ['q' => 'node/1', 'p' => 'a+b'], 'https://example.com/?q=node%2F1&p=a%2Bb', TRUE],
+      [
+        'https://example.com/route-to-normalize',
+        [],
+        'https://example.com/route-to-normalize',
+        FALSE,
+      ],
+      [
+        'https://example.com/route-to-normalize',
+        ['key' => 'value'],
+        'https://example.com/route-to-normalize?key=value',
+        FALSE,
+      ],
+      ['https://example.com/index.php/',
+        ['q' => 'node/1'],
+        'https://example.com/?q=node%2F1',
+        TRUE,
+      ],
+      ['https://example.com/index.php/',
+        ['q' => 'node/1', 'p' => 'a+b'],
+        'https://example.com/?q=node%2F1&p=a%2Bb',
+        TRUE,
+      ],
     ];

The existing code is more readable.

    * @return \Drupal\Core\Routing\UrlGeneratorInterface|\PHPUnit\Framework\MockObject\MockObject
+   *   URl generator.

URl is misspelled.

The status is still Needs work, since no new patch nor new MR has been provided. When that is done, the status becomes Needs review, not Active.

It was noted in #15 but there was already a mostly-duplicate issue 📌 Maintaining drupal coding standards Fixed -- and as I've just re-rolled the patch for that, I'm marking this one postponed to prevent any more duplication of effort.

I'm not closing this as a duplicate issue because I've noted some phpcs issues which I did not fix in that other patch; namely:

* \Drupal calls should be avoided in classes, use dependency injection instead
* unserialize() is insecure unless allowed classes are limited. Use a safe format like JSON or use the allowed_classes option.

I think once the coding standards issue has been resolved, this one can be re-opened to address any remaining phpcs concerns.

alexpott → changed the visibility of the branch 8.x-1.x to hidden.

alexpott → changed the visibility of the branch 3324524-drupal-coding-standards to hidden.

I've rolled a new branch on top of 📌 Making coding QA stages fail and fix or add baselines as necessary Active as that would be good to land first and then fixed all the reported PHPCS issues. Going to leave at needs work so we can merge the MR after the other issue lands.

Merged in 8.x-1.x and now we're good to go! Green PHPCS run and will error on fail.

alexpott → changed the visibility of the branch 3540774-making-coding-qa-phpcs to hidden.

We need to land 📌 Drop Drupal 9 and PHP 7 support Active first.

that is in.

I considered if we should resolve some phpstan issues on changed lines, but they all seem to be non-trivial, like the removed exception that indicates dead/broken code, not sure if that was ever valid and what changed, so lets not.

The part that's not clear for me yet is the hook changes, I know some hooks don't exist anymore, but not sure how that's related to phpcs and didn't see any explanation or discussion on that here.

The redirect.api.php stuff is changed because we've got PHPCS issues in the file

FILE: /Volumes/dev/sites/drupal8alt.dev/modules/redirect/redirect.api.php
-------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 6 LINES
-------------------------------------------------------------------------
 101 | ERROR | Missing parameter type
 103 | ERROR | Missing parameter type
 115 | ERROR | Missing parameter type
 117 | ERROR | Missing parameter type
 119 | ERROR | Missing parameter type
 142 | ERROR | Missing parameter type
-------------------------------------------------------------------------

And it seems silly to fix things that are just not relevant anymore.

Re the PHPStan issues - I think we should address all of that in the follow-up - 🐛 PHPStan issue report in gitlab Active

More info on the .api.php changes... there are 3 hooks where this MR is remeving docs:

• hook_redirect_load: the signature is wrong and this better documented by hook_ENTITY_TYPE_load() in entity.api.php
• hook_redirect_load_by_source_alter: does not exist anymore
• hook_redirect_prepare: does not exist anymore and is replaced by hook_ENTITY_TYPE_prepare_form (which is documented in entity.api.php)

Merged, thanks. if this gets too tedious to maintain with minor core phpcs changes I might change the flag in the future.