Drupal best practices and coding standards

1. +++ b/src/Controller/TokenDevelController.php
   @@ -88,11 +88,12 @@ class TokenDevelController extends ControllerBase {
        ];
   -//    foreach ($tree as $token => $token_info) {
   -//      if (!isset($token_info['value']) && !empty($token_info['parent']) && !isset($tree[$token_info['parent']]['value'])) {
   -//        continue;
   -//      }
   -//    }
   +
   +/*    foreach ($tree as $token => $token_info) {
   +     if (!isset($token_info['value']) && !empty($token_info['parent']) && !isset($tree[$token_info['parent']]['value'])) {
   +       continue;
   +     }
   +   } */
    
   

   I doubt that formatting is correct, and unsure why it is commented out, but I'd rather just remove it if not required anymore.

2. +++ b/src/Element/TokenTreeTable.php
   @@ -70,7 +70,7 @@ class TokenTreeTable extends Table {
            if (!empty($token_info['restricted']) && empty($element['#show_restricted'])) {
              continue;
            }
   -        if ($element['#skip_empty_values'] && empty($token_info['value']) && !empty($token_info['parent']) && !isset($tree[$token_info['parent']]['value'])) {
   +        if ($element['#skip_empty_values'] && empty($token_info['value']) && !empty($token_info['parent'])) {
              continue;
   

   this is not the same check, why change this?

3. +++ b/src/Tests/TokenMenuTest.php
   @@ -99,7 +99,7 @@ class TokenMenuTest extends TokenTestBase {
        $node = $this->drupalCreateNode();
    
        // Allow main menu for this node type.
   -    //$this->config('menu.entity.node.' . $node->getType())->set('available_menus', array('main-menu'))->save();
   +    /* $this->config('menu.entity.node.' . $node->getType())->set('available_menus', array('main-menu'))->save(); */
    
   

   same here, if really not needed then lets remove this. This would also be a valid inline comment, it's just missing a space.

4. +++ b/token.install
   @@ -220,30 +220,30 @@ function token_update_token_text($text, $updates = array(), $leading = '[', $tra
   -  //  }
   -  //}
   +/*   $token_info = array();
   +  foreach (module_implements('token_info') as $module) {
   +   $module_token_info = module_invoke($module, 'token_info');
   

   same here, this doesn't seem formatted correctly and I'd rather fix the code instad of just fixing the way it is commented out, that doesn't bring us forward.

5. +++ b/token.tokens.inc
   @@ -67,7 +67,7 @@ function token_token_info_alter(&$info) {
    
      // Add [token:url] tokens for any URI-able entities.
      $entities = \Drupal::entityTypeManager()->getDefinitions();
   -  foreach ($entities as $entity => $entity_info) {
   +  foreach ($entities as $entity_info) {
        // Do not generate tokens if the entity doesn't define a token type or is
   

   weird variable names, we could rename it to $entity_types and $entity_type while touching it anyway.

fixed as per #4

Tested the patch and it fixed coding standard and issues that @Berdir has found.

Patch submitted in #5 still has some issue.

FILE: /var/www/html/drupal/modules/token/token.install
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 246 | WARNING | There must be no blank line following an inline
     |         | comment
     |         | (DrupalPractice.Commenting.CommentEmptyLine.SpacingAfter)
----------------------------------------------------------------------


FILE: /var/www/html/drupal/modules/token/src/Tests/TokenMenuTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 102 | WARNING | There must be no blank line following an inline
     |         | comment
     |         | (DrupalPractice.Commenting.CommentEmptyLine.SpacingAfter)
----------------------------------------------------------------------


FILE: ...var/www/html/drupal/modules/token/src/Element/TokenTreeTable.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 73 | WARNING | Variable $tree is undefined.
    |         | (DrupalPractice.CodeAnalysis.VariableAnalysis.UndefinedVariable)
----------------------------------------------------------------------

This is the patch and interdiff.

It would be great if someone let me know regarding this warning so i will fix it.

FILE: ...var/www/html/drupal/modules/token/src/Element/TokenTreeTable.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 73 | WARNING | Variable $tree is undefined.
    |         | (DrupalPractice.CodeAnalysis.VariableAnalysis.UndefinedVariable)
----------------------------------------------------------------------

I was unable to apply the patch due to changes in the version. Thus updating the rerolling patch.

Updating the patch with the solved changes with the interdiff.

Please review the patch. Thanks

Patch failed to apply. Needs reroll.

#15 didn't apply clean, i rerolled it and worked on other files.

Fixed all empty doc blocks and other CS details. Diffed against 8.x-1.x.

Needs review.

Silly me, forgot to unassign. Sorry.

Corrects "TokenTestTrait" under tests/src/Functional/TokenTestBase.php

Needs review.

Corrects route access for TreeTest data retrieval and wrong variable name on HelpPageTest

Now everything might pass (tests were passing on local, but failing here. So i worked against DO test results).

Needs review :)

I run all tests and phpcs command to find any erros. No indentation problems were found and tests are running well.

No crashes happened during my review.

I'm moving it to RTBC.

Should this patch be needed for release 8.x-1.11 because I am getting a failure when it tries to patch via composer and when I do it manually

-> patch -p1 <2933031-20.patch
patching file README.md
patching file css/jquery.treetable.css
patching file css/token.css
patching file src/Controller/TokenDevelController.php
patching file src/Controller/TokenTreeController.php
patching file src/Element/TokenTreeTable.php
Hunk #3 succeeded at 113 (offset -4 lines).
Hunk #4 succeeded at 162 (offset -4 lines).
Hunk #5 succeeded at 182 (offset -4 lines).
patching file src/MenuLinkFieldItemList.php
patching file src/Plugin/Derivative/DevelLocalTask.php
patching file src/Routing/RouteSubscriber.php
patching file src/Token.php
patching file src/TokenEntityMapper.php
patching file src/TokenEntityMapperInterface.php
patching file src/TokenFieldRender.php
patching file src/TokenInterface.php
patching file src/TokenServiceProvider.php
patching file src/TreeBuilder.php
patching file src/TreeBuilderInterface.php
patching file tests/modules/token_module_test/token_module_test.module
patching file tests/modules/token_module_test/token_module_test.routing.yml
patching file tests/modules/token_module_test/token_module_test.tokens.inc
patching file tests/src/Functional/TokenBlockTest.php
patching file tests/src/Functional/TokenCurrentPageTest.php
patching file tests/src/Functional/TokenFieldUiTest.php
patching file tests/src/Functional/TokenMenuTest.php
patching file tests/src/Functional/TokenTestTrait.php
patching file tests/src/Functional/TokenURLTest.php
patching file tests/src/Functional/TokenUserTest.php
patching file tests/src/Functional/Tree/HelpPageTest.php
patching file tests/src/Functional/Tree/TokenTreeTestTrait.php
patching file tests/src/Functional/Tree/TreeTest.php
Hunk #3 FAILED at 98.
1 out of 5 hunks FAILED -- saving rejects to file tests/src/Functional/Tree/TreeTest.php.rej
patching file tests/src/Functional/UrlTest.php
patching file tests/src/Kernel/ArrayTest.php
patching file tests/src/Kernel/BookTest.php
patching file tests/src/Kernel/CommentTest.php
patching file tests/src/Kernel/DateTest.php
patching file tests/src/Kernel/EntityTest.php
patching file tests/src/Kernel/FieldTest.php
patching file tests/src/Kernel/FileTest.php
patching file tests/src/Kernel/LanguageTest.php
patching file tests/src/Kernel/NodeTest.php
patching file tests/src/Kernel/RandomTest.php
patching file tests/src/Kernel/TaxonomyTest.php
patching file tests/src/Kernel/UnitTest.php
patching file tests/src/Kernel/UrlTest.php
patching file token.install
patching file token.module
patching file token.pages.inc
patching file token.tokens.inc

--- tests/src/Functional/Tree/TreeTest.php
+++ tests/src/Functional/Tree/TreeTest.php
@@ -98,7 +103,10 @@

// Request with show_restricted set to TRUE to show restricted tokens and
// check for them.
- $this->drupalGet($this->getTokenTreeUrl(['token_types' => ['user'], 'show_restricted' => TRUE]));
+ $this->drupalGet($this->getTokenTreeUrl([
+ 'token_types' => ['user'],
+ 'show_restricted' => TRUE,
+ ]));
$this->assertEquals('MISS', $this->drupalGetHeader('x-drupal-dynamic-cache'), 'Cache was not hit');
$this->assertTokenInTree('[user:one-time-login-url]', 'user');
$this->assertTokenInTree('[user:original:cancel-url]', 'user--original');

Rerolled #21.

Sorry, i made a mistake on the last patch.

Kindly review it.

I'll be reviewing.

I applied #27 patch and no more PHPCS errors.

#27 looks good for me. after seeing #29 no PHPCS errors.
Should we consider moving to RTBC to Fixed!

On my 9.5 Beta 2 system when I try to apply patch 27 now it applies correctly however after I do the computer update -W and drush updb I then run drush cr to flush the cache and I get the following:

Fatal error: Uncaught Error: Call to undefined method Drupal\Core\StringTranslation\TranslationManager::t() in /home/sitename/public_html/modules/contrib/token/src/Plugin/Derivative/DevelLocalTask.php:64
Stack trace:
#0 /home/sitename/public_html/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php(101): Drupal\token\Plugin\Derivative\DevelLocalTask->getDerivativeDefinitions(Array)
#1 /home/sitename/public_html/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php(87): Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDerivatives(Array)
#2 /home/sitename/public_html/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php(285): Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinitions()
#3 /home/sitename/public_html/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php(175): Drupal\Core\Plugin\DefaultPluginManager->findDefinitions()
#4 /home/sitename/public_html/core/lib/Drupal/Core/Menu/LocalTaskManager.php(181): Drupal\Core\Plugin\DefaultPluginManager->getDefinitions()
#5 /home/sitename/public_html/modules/contrib/webform/webform.module(521): Drupal\Core\Menu\LocalTaskManager->getDefinitions()
#6 /home/sitename/public_html/core/lib/Drupal/Core/Extension/ModuleHandler.php(562): webform_menu_links_discovered_alter(Array, NULL, NULL)
#7 /home/sitename/public_html/core/lib/Drupal/Core/Menu/MenuLinkManager.php(166): Drupal\Core\Extension\ModuleHandler->alter('menu_links_disc...', Array)
#8 /home/sitename/public_html/core/lib/Drupal/Core/Menu/MenuLinkManager.php(189): Drupal\Core\Menu\MenuLinkManager->getDefinitions()
#9 /home/sitename/public_html/core/lib/Drupal/Core/EventSubscriber/MenuRouterRebuildSubscriber.php(82): Drupal\Core\Menu\MenuLinkManager->rebuild()
#10 /home/sitename/public_html/core/lib/Drupal/Core/EventSubscriber/MenuRouterRebuildSubscriber.php(70): Drupal\Core\EventSubscriber\MenuRouterRebuildSubscriber->menuLinksRebuild()
#11 [internal function]: Drupal\Core\EventSubscriber\MenuRouterRebuildSubscriber->onRouterRebuild(Object(Drupal\Component\EventDispatcher\Event), 'routing.route_f...', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#12 /home/sitename/public_html/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(142): call_user_func(Array, Object(Drupal\Component\EventDispatcher\Event), 'routing.route_f...', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#13 /home/sitename/public_html/core/lib/Drupal/Core/Routing/RouteBuilder.php(197): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object(Drupal\Component\EventDispatcher\Event), 'routing.route_f...')
#14 /home/sitename/public_html/core/lib/Drupal/Core/ProxyClass/Routing/RouteBuilder.php(83): Drupal\Core\Routing\RouteBuilder->rebuild()
#15 /home/sitename/public_html/core/includes/common.inc(587): Drupal\Core\ProxyClass\Routing\RouteBuilder->rebuild()
#16 /home/sitename/public_html/core/includes/utility.inc(41): drupal_flush_all_caches(Object(Drupal\Core\DrupalKernel))
#17 /home/sitename/vendor/drush/drush/src/Commands/core/CacheCommands.php(227): drupal_rebuild(Object(Composer\Autoload\ClassLoader), Object(Symfony\Component\HttpFoundation\Request))
#18 [internal function]: Drush\Commands\core\CacheCommands->rebuild(Array)
#19 /home/sitename/vendor/consolidation/annotated-command/src/CommandProcessor.php(257): call_user_func_array(Array, Array)
#20 /home/sitename/vendor/consolidation/annotated-command/src/CommandProcessor.php(212): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback(Array, Object(Consolidation\AnnotatedCommand\CommandData))
#21 /home/sitename/vendor/consolidation/annotated-command/src/CommandProcessor.php(176): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter(Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
#22 /home/sitename/vendor/consolidation/annotated-command/src/AnnotatedCommand.php(350): Consolidation\AnnotatedCommand\CommandProcessor->process(Object(Symfony\Component\Console\Output\ConsoleOutput), Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
#23 /home/sitename/vendor/symfony/console/Command/Command.php(255): Consolidation\AnnotatedCommand\AnnotatedCommand->execute(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#24 /home/sitename/vendor/symfony/console/Application.php(1027): Symfony\Component\Console\Command\Command->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#25 /home/sitename/vendor/symfony/console/Application.php(273): Symfony\Component\Console\Application->doRunCommand(Object(Consolidation\AnnotatedCommand\AnnotatedCommand), Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#26 /home/sitename/vendor/symfony/console/Application.php(149): Symfony\Component\Console\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#27 /home/sitename/vendor/drush/drush/src/Runtime/Runtime.php(124): Symfony\Component\Console\Application->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#28 /home/sitename/vendor/drush/drush/src/Runtime/Runtime.php(51): Drush\Runtime\Runtime->doRun(Array, Object(Symfony\Component\Console\Output\ConsoleOutput))
#29 /home/sitename/vendor/drush/drush/drush.php(72): Drush\Runtime\Runtime->run(Array)
#30 /home/sitename/vendor/drush/drush/includes/preflight.inc(18): require('/home/pls/prepp...')
#31 phar:///usr/local/bin/drush/bin/drush.php(159): drush_main()
#32 /usr/local/bin/drush(10): require('phar:///usr/loc...')
#33 {main}
thrown in /home/sitename/public_html/modules/contrib/token/src/Plugin/Derivative/DevelLocalTask.php on line 64
[warning] Drush command terminated abnormally.

This does not happen if I do not have 27 applied.

Coding standard improvements must be provided as merge requests now, so that we can verify it using GitlabCI.

Additionally, this is way too big as a single patch and overlaps with many other issues. This is impossible to review and needs to be split up into issues for specific changes or groups of related changes.