[D8] User History

hello, jlscott, there is still one warning in the preview checklist. When I run the PHPCS command on my local, I got multiple phpcbf and deprecation issues.

@Rajeshreeputra The review should describe what needs to be changed. Saying there are deprecation issues to be fixed isn't sufficient. Even if the description doesn't cover all the issues, it's better than just saying there are deprecation issues.

in user_history.restore.inc $max_line_length & $field_definitions variables are unused

  // Set the max allowed line length.
  $max_line_length = 2048;

// Get a list of fields attached to the user_history entity.
      $field_definitions = \Drupal::service('entity_field.manager')->getFieldDefinitions('user_history', 'user_history');

in user_history.module $field_type is unused

if ($field_definition instanceof FieldConfig) {
          // $field_type is not currently used.
          $field_type = $field_definition->getType();
        }

in user_history.batch.inc $field_type is used at multiple places

// $field_type is not currently used.
        $field_type = $field_definition->getType();

if you are not using these fields for now and added for future development, then you can keep as commented.

• What follows is a quick review of the project; it doesn't mean to be complete
• For every point, I didn't make a complete list of each line that should be fixed, but an example of what needs to be changed
• The review points report what should be changed to follow the Drupal coding standards, correctly use the Drupal API, or avoid any (possible) security issue; if a review point isn't about one of those topics, the point makes that clear

      $requirements['user_history'] = [
        'title' => t('User history'),
        'value' => t($message_text, $message_args),
        'severity' => $severity,
      ];

The first argument of t() needs to be a literal string not a dynamic value as in this case, where $message_text is a concatenation of two strings that change basing on runtime conditions.
In this case it's not a security issue, as the string values don't come from user input, but it's a wrong use of the Drupal API: The documentation for t() explains the first argument needs to be a literal string, not only to avoid possible security issues, but also because the code that extracts the strings to translate only extracts literal strings, such as 'User history'. If there is a variable part, that needs to be included using placeholders.

Concatenating the strings after passing them to t() resolves the issue.

$message_text = t('The user_history module was initialised on %date. There are %users users defined, along with %history history records currently in the database.', ['%date' => $install_date, '%users' => $count_users, '%history' => $count_history]);

$batch_msg = t('The user history records need to be initialised. <a href=":initialise_form">Initialise records</a>.', ['initialise_form' => Url::fromRoute('user_history.batch_install_form')->toString()]);

// Now the strings can be concatenated.
$requirements['user_history'] = [
  'title' => t('User history'),
  'value' => ($message_text . ' ' . $batch_msg,
  'severity' => $severity,
];

/**
 * Provides a dummy form to prevent deleting user_history entities.
 *
 * @ingroup user_history
 */
class UserHistoryDeleteForm extends FormBase {

  /**
   * {@inheritdoc}
   */
  public function getFormId() {
    return 'user_history_delete';
  }

  /**
   * {@inheritdoc}
   */
  public function buildForm(array $form, FormStateInterface $form_state) {
    return $form;
  }

  /**
   * {@inheritdoc}
   */
  public function submitForm(array &$form, FormStateInterface $form_state) {
  }

}

If the entity isn't supposed to be edited or deleted via form, it could be better to implement an access handler that doesn't allow users to create, edit, and delete the entity. That is what Drupal core does with the entity implemented with ContentModerationState. Code can still get a list of those entities, using code similar to the following one.

$storage = \Drupal::entityTypeManager()->getStorage('content_moderation_state');
$ids = $storage->getQuery()
  ->accessCheck(FALSE)
  ->condition('content_entity_type_id', $entity
  ->getEntityTypeId())
  ->condition('content_entity_id', $entity
  ->id())
  ->condition('workflow', $moderation_info
  ->getWorkflowForEntity($entity)
  ->id())
  ->condition('content_entity_revision_id', $entity->getLoadedRevisionId())->allRevisions()->execute();

I would not consider this a misunderstanding of how to use the Drupal API. In Drupal, it's preferred not to show an empty form to users who aren't allowed to use it.

Thank you for your contribution! I am going to update your account.

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.