[D8] Go Back

Review of the 8.x-1.x branch (commit c6aeaa3):

• PAReview result is almost clean, only writing of test is recommended: https://pareview.sh/pareview/https-git.drupal.org-project-go_back.git.
• Nice README.txt file - please fix only useless "</ul>" and space after dot ("configuration.You").
• Package inside go_back.info.yml is set to "Custom" - isn't there some better category? If not, I would use "User interface" or "Other" instead.
• Inside repository, there is branch named "@branch" - probably better to delete as it doesn't contain any commits.
• File go_back.css contains absolute paths - please fix them to be relative ("../images/back.png")
• Your hook_theme provides "block__go__back" template - it should be renamed to "block__go_back" (see "system_theme()").
• Block GoBack.php is having some minor issues:
  • Condition "if ($config[$bundle]['quick_display'] == '1')" is doubled (lines 177 and 178).
  • Also "$urlb = $_SERVER['HTTP_REFERER'];" is doubled (lines 180 and 182).
  • Basically whole code block of lines 185-194 and 198-207 are duplicates without any reason.
  • It would be good to autoformat (few extra newlines).
  • Fix coding standards (usage of double quotes for strings).
  • Add some inline comments into "build()" function.
  • I would also replace comparing like "$var == '1'" by "!empty($var)".

Review of the 8.x-1.x branch (commit 77b610d):

Module passes Drupal and DrupalPractice coding standards (on my local machine).

1. I noticed in GoBack.php on line 186 if statement with comment inside. It looks like a statement with empty body:

if (strcmp($urlb, '') !== 0) {
            // Check if the url is from our site.
          }

Is it necessary there?

2. Also, code on line 244 would throw an exception if entity type doesn't exist or storage handler couldn't be found. Should those exceptions be caught?
This is from doc comments in EntityTypeManagerInterface.php
* @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
* Thrown if the entity type doesn't exist.
* @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException
* Thrown if the storage handler couldn't be loaded.

3. Line 185: I would add check whether $_SERVER['HTTP_REFERER'] exists since you wrote in comments that it could be from another site.
There might be chance HTTP_REFERRER is not set by some domains. (I don't think it breaks code, since it would be just in $_SERVER array).

Thank you for your efforts.

No problem, saesa.
As I wrote, for me your code looks fine.
About first notice with empty if block: since you code jumps to else block, what if you change it like that:

          if (strcmp($urlb, '') === 0) {
            // Check if the url is from our site.
 // If the url is not from our site, take the url by default.
            if (substr($config[$bundle]['go_back'], 0, 1) == '/') {
              /*
              Transforms the url into a readable URL for Drupal
              and add the interface language.
               */
              $url = Url::fromUri('base:' . $config[$bundle]['go_back'], [
                'language' => $this->languageInt->getLanguage($langcode),
              ]);
              $path = $url->toString();
              $urlb = $path;
            }
          }

It should work in the same way as you current code.

Saesa, please check attached patch. As I mentioned, it is not blocker, and in my opinion, module can be pushed to security coverage policy.
Can I myself change issue's status to fixed (and tested)?

      $form[$content_type]['quick_display'] = [
        '#type' => 'checkbox',
        '#title' => $this->t("Active button in") . ' ' . $content_type,
        '#options' => $content_type,
        '#default_value' => $config[$content_type]['quick_display'],
      ];

String to translate should use placeholders to include the dynamic part of the string. In this way, translators have more context to translate the string, considering that in some language the translation of Active button in could change basing on the next word/phrase.

Placeholders aren't translated.
If you look at Drupal core for similar code, you won't find code that concatenates a translated string to a not translated string. In those cases, a placeholder is used inside the translated string.

Yes, I mean that and similar code the module is using. Compare it with the following code took from NodeController.php.

return $this->t('Revision of %title from %date', [
  '%title' => $node->label(),
  '%date' => format_date($node->getRevisionCreationTime()),
]);

For more information on the placeholder types, see FormattableMarkup::placeholderFormat().

As side note, even concatenating translated string should be avoided.

// Wrong code.
$header = $this->t('List') . ' ' .  $this->t('of') . ' ' . $this->t('files to delete'); 

The code should be similar to the following one. A checkbox form element doesn't use #options.

$form[$content_type]['quick_display'] = [
  '#type' => 'checkbox',
  '#title' => $this->t("Active button in %content_type", ['%content_type' => $content_type]),
  '#default_value' => $config[$content_type]['quick_display'],
  '#attributes' => [
    'placeholder' => $this->t("Active button in %content_type", ['%content_type' => $content_type]),
  ],
];

Thank you for your contribution!

I am going to update your account so you can opt into security advisory coverage now.

These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.

Thank you, also, for your patience with the review process.

Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

Adding credits.