[1.0.x] Clear base64 image

Hello,

I'm currently working on the module and I fix many of error of PHPCS issues.
But try to replace \Drupal in the function replace(), make error of datable not serializable.

@apaderno This account is a shared account.

Hello,

I fix all error of phpcs, there is no more error.

Thanks

1. FILE: clear_base64_image.libraries.yml

You should remove this file since it is empty.

2. Replace all occurrences of 'sites/default/files/' with dynamic path since some Drupal projects might be using other location for public file directory.

src/Controller/ClearBase64ImageController.php
src/Controller/ClearBase64ImageReplaceController.php
src/Form/ClearBase64imageSettings.php

Shared accounts cannot be used to make commits on drupal.org repositories. That is what the Drupal.org Terms of Service → says. (Emphasis is mine.)

If you are sharing your user account with multiple people (e.g. as your “official” organization account), you are not allowed to do the following using this account:

• Commit code to Git repositories on the Website
• Create any node except for organization, case study or project nodes
• Comment on nodes

Each developer needs to create a drupal.org account that is always used from the same developer.

@apaderno Okay thanks for the information. But if i add my personal account in the maintainers of the project, this work ?

I really want to share my module to the community, i think this module can help a lot of people.

If you create your own account, add that to the project maintainers, and start making commits with that account and only that account, then yes.

Thanks for the information.
@apaderno I have had my personal account in the project maintainers. I have also making commit with that account.

@vishal.kadam I Check for the modification.

I will review the project in the next 12/14 hours.
Please do not make reviews, since there is something that needs to be changed in the project code; before reporting that, it is useless to suggest other changes.

The ClearBase64ImageController and ClearBase64ImageReplaceController classes must be replaced by a form class with a form submission handler that handles nodes.

src/Controller/ClearBase64ImageReplaceController.php

    foreach ($results as $result) {
      $node_storage = $this->EntityTypeManager()->getStorage('node');
      $nid = $result->nid;
      $node = $node_storage->load($nid);

      $description = $node->body->value;

      $xpath = new \DOMDocument();
      $xpath->loadHTML($description);

      $img = $xpath->getElementsByTagName('img');

      foreach ($img as $link) {

        $imgSrc = $link->getAttribute('src');

        $linkHref = explode(',', $imgSrc);

        $linkHref = end($linkHref);

        // If src is base64.
        if (base64_decode($linkHref, TRUE)) {

          $count++;

          $decode = base64_decode($linkHref);

          // Get title.
          $imgTitle = $link->getAttribute('title');

          // Get informations about the image.
          $f = finfo_open();
          $mime_type = finfo_buffer($f, $decode, FILEINFO_MIME_TYPE);
          $mime_type = explode('/', $mime_type)[1];

          if ($getImgType) {
            $mime_type = 'png';
          }
          if ($getImgType == 2) {
            $mime_type = 'jpg';
          }

          $date = getdate();
          $date = $date['seconds'] . $date['minutes'] . $date['hours'] . $date['mday'] . $date['mon'] . $date['year'];

          $new_file = empty($imgTitle) ? $directory . $node->nid->value . '-' . substr($linkHref, 120, 10) . '-' . $date . '.' . $mime_type : $directory . $imgTitle . '.' . $mime_type;

          file_save_data($decode, $new_file, $file_exists);
          $arrayFiles[$count][$nid] = $new_file;
          $arrayFiles[$count]['src'] = $imgSrc;

        }
      }

Loading all the nodes, which could be potentially be more than 100, needs to be done using the batch API. Furthermore, using the batch API, nodes are not first all loaded, but they are loaded one for each iteration of the batch API processing callback.

   $arrayFile[$key] = str_replace('public://', 'sites/default/files/', $arrayFile[$key]);

To get the real path of a file stored in the public:// directory, that is wrong code. The correct code needs to use FileSystem::realpath().

$query->condition('body_value', '%' . $db->escapeLike('data:image/', 'base64') . '%', 'LIKE');

escapeLike() requires a single argument: the string to escape.

src/Controller/ClearBase64ImageController.php

    $header = [
      ['data' => ''],
      ['data' => $this->t('Image')],
      ['data' => $this->t('Node')],
    ];

The header is simply an array of labels, such as in the following code, taken by a Drupal core module.

  $header = [$this->t('Tag Description'),
    $this->t('You Type'),
    $this->t('You Get'),
  ];

'data' is only used for rows.

There are other changes to do, but it is better to first do these changes, at least because these changes require removing two classes and implement a new one.

Hello,
thanks for you review.
I check for the modification.

Hello,
I have applied all review.
I have change ClearBase64ImageController and ClearBase64ImageReplaceController to a Form class.

I have change "public://" with :
file_url_transform_relative(file_create_url($new_file)) because FileSystem::realpath() not return valid path.

I have remove data form $header.

Also change :
$db->escapeLike('data:image/', 'base64')
to $db->escapeLike('data:image/')

Hi @apaderno I can also add things to the review if it's OK.

Indeed, it is more than OK. I forgot to remove myself from the Assigned field.

Thanks,

From my point of view, the hook_help should be a bit more specific:

function clear_base64_image_help($route_name, RouteMatchInterface $route_match) {
  switch ($route_name) {
    case 'help.page.clear_base64_image':
      $output = '';
      $output .= '<h3>' . t('About') . '</h3>';
      $output .= '<h3>' . t('Configuration') . '</h3>';
      return $output;
  }
}

In /src/Form/ClearBase64ImageList.php line 121 maybe should use render array rather than build the link.

Markup::create('<a href="' . $data['description'] . '" target="_blank">  Display </a>'),

same file, lines 318 and next

      if ($getImgType) {
        $mime_type = 'png';
      }
      if ($getImgType == 2) {
        $mime_type = 'jpg';
      }

Seems to override once and again the same variable. Also I think some constants should be used in oder to make the code easier to understand.

Line 328

$new_file = empty($imgTitle) ? $directory . $node->nid->value . '-' . substr($linkHref, 120, 10) . '-' . $date . '.' . $mime_type : $directory . $imgTitle . '.' . $mime_type;

Maybe needs some explanaition in comments, as the link substr($linkHref, 120, 10) really depends on variable factors.

In general for that file we should not mix camel case and snake case in same file:
https://www.drupal.org/docs/develop/standards/php/php-coding-standards#s... →

Hello,
I have applied all review.

Update the hook_help with more information about the module.

Change the Markup::create with :
new FormattableMarkup('<a href="@src" > Display </a>', ['@src' => $data['description']]),

Change variable $mime_type with constant.

I transform the part of code ( Line 328 ) with :

      if (empty($imgTitle)) {
        // If image don't have title.
        // Generate title with nid, parts of the base64 and the current date.
        $new_file = $directory . $node->nid->value . '-' . substr($linkHref, 120, 10) . '-' . $date . '.' . $imgType;
      }
      else {
        // If image already have title.
        // The file get the name of the image title.
        $new_file = $directory . $imgTitle . '.' . $imgType;
      }

I think is more easy to understand with that.

Thanks for your review.

• The following points are just a start and don't necessarily encompass all of the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

src/Form/ClearBase64ImageList.php

/**
 * Controller routines for manage and replace page.
 */
class ClearBase64ImageList extends ConfigFormBase {

The short description still describes the class as a controller, but it is a form class. The description is not valid for a form since forms do not replace pages.

  /**
   * The configuration factory.
   *
   * @var \Drupal\Core\Config\ConfigFactoryInterface
   */
  protected $configFactory;

The $configFactory property is already defined from a parent class; there is no need to redefine it.
Child classes need to initialize the parent property with setConfigFactory(); when they need to read configuration, they need to use config().

    foreach ($datas as $data) {

      $rows[$i] = [

        new FormattableMarkup('<a href="@src" > Display </a>', ['@src' => $data['description']]),
        $data['descriptionSubstr'],
        $data['title'],
        $data["id"],

      ];

      $i++;

    }

Strings shown in the user interface must be translatable. FormattableMarkup is not the class to translate strings.
The correct placeholder for URLs starts with : as shown in the documentation for FormattableMarkup::placeholderFormat(). That is the placeholder used also by Drupal core code for URLs that take to drupal.org.

    $form['table'] = [
      '#type' => 'tableselect',
      '#header' => $header,
      '#options' => $rows,
      '#empty' => 'No data',
    ];

For the same reason, the value assigned to `'#empty'` must be a translatable string.

    // Display directory.
    $message = $this->t('Target directory : @directory', ['@directory' => $public_folder . $directory]);
    $messenger = $this->messenger();
    $messenger->addMessage($message, $messenger::TYPE_WARNING);

The target directory is shown in a warning message. That is eventually an informative message, but it is not necessary to show it, since the destination directory has been chosen by users in the module settings form.

    // Display warning message if directory is not writable.
    if (!$writable) {

      $message = $this->t('The directory is not writable');
      $messenger = $this->messenger();
      $messenger->addMessage($message, $messenger::TYPE_WARNING);

    }

The form shows that error when the directory is not writable, but the form can still be submitted. In that case, the error should be shown in the form submission handler; the error message should also be clearer, or users will probably keep clicking on the Convert button wondering why it does not work.

  /**
   * Get base64 image from database.
   */

The description is too generic. It also misses the return value description.

    $db = $this->database;
    $query = $db->select('node_field_data', 'n');
    $query->addField('n', 'nid');

    $query->leftJoin('node__body', 'body', 'body.entity_id = n.nid');
    $query->condition('body_value', '%' . $db->escapeLike('data:image/') . '%', 'LIKE');

    $results = $query->execute()->fetchAll();

To query entities, it is better to use the object returned by $this->entityTypeManager->getStorage('node')->getQuery('AND') which implements \Drupal\Core\Entity\Query\QueryInterface. In this case, the operator is `'CONTAINS'`.

  public function submitForm(array &$form, FormStateInterface $form_state) {

    $selected_names = [];
    $fields = array_filter($form_state->getValue('table'));

    $datas = $this->getInfoImage();

    foreach ($fields as $field) {
      $selected_names[] = $datas[$field - 1];
    }

    $batch = [
      'title' => $this->t('Processing'),
      'operations' => [],
      'finished' => '\Drupal\clear_base64_image\Form\ClearBase64ImageList::updateMessage',
    ];

    foreach ($selected_names as $selected) {

      $batch['operations'][] = [
        '\Drupal\clear_base64_image\Form\ClearBase64ImageList::replace',
        [$selected],
      ];

    }

    batch_set($batch);

  }

The code is still loading all the nodes to process before running the batch. That is not how the batch API is used. The batch operation callback needs to load few nodes and process those nodes, otherwise the operation is not done in batches.
An example of batch operation callback is _node_access_rebuild_batch_operation(). That is procedural code, but similar code can be used in a method.

if (base64_decode($linkHref, TRUE)) {

To verify $linkHref is really Base64-encoded, the code to use is if (base64_encode(base64_decode($linkHref, true)) === $linkHref) {.

    if ($success) {
      $message = t('Success');
    }
    else {
      $message = t('Finished with an error.');
    }

    $messenger = \Drupal::messenger();
    $messenger->addMessage($message);

    $path = Url::fromRoute('clear_base64_image.manage');

    return new RedirectResponse($path->toString());

Since the batch operations are executed in a form, there is no need to redirect. That code would not work, since the value returned by the batch finished callback is not used.

src/Form/ClearBase64imageSettings.php

  public function validateForm(array &$form, FormStateInterface $form_state) {

    $public_folder = $this->fileSystem->realpath("public://") . "/";

    if (!file_exists($public_folder . $form_state->getValue('directory'))) {

      $form_state->setErrorByName('directory', $this->t("This path does not exist"));

    }

    if (substr($form_state->getValue('directory'), -1) != "/") {

      $form_state->setErrorByName('directory', $this->t("The path must finish with a <b> ' / ' </b>"));

    }

  }

The code should also check the directory is a directory and not a file, and the directory is writable.

   $public_folder = $this->fileSystem->realpath("public://") . "/";

    $this->messenger()->addStatus($this->t('The directory is @directory', ['@directory' => $public_folder . $form_state->getValue('directory')]));

It does not make sense to show to users which directory they selected, since they entered that value. The only useful message is already given by the parent method.

$config = $this->config('clear_base64_image.settings');

// Set the submitted configuration setting.
$config->set('directory', $form_state->getValue('directory'));
$config->set('type', $form_state->getValue('type'));
$config->save();

Methods concatenation is allowed.

$this->config('clear_base64_image.settings')
  ->set('directory', $form_state->getValue('directory'))
  ->set('type', $form_state->getValue('type'))
  ->save();

The comment is not necessary, since it just says what is already clear from the code.

clear_base64_image.routing.yml

clear_base64_image.replace:
  path: 'admin/config/media/clear_base64_image-blocks/replace'
  defaults:
    _controller: '\Drupal\clear_base64_image\Form\ClearBase64ImageList::replace'
  requirements:
    _permission: 'administer clear_base64_image'
  options:
    _admin_route: TRUE

Since the files are created from a form submission handler, that route is not necessary. Users just need to access the form.

In general, the code contains empty lines that should not be there, for example in the following cases.

  public function getImgDatabase() {

    $db = $this->database;
    $query = $db->select('node_field_data', 'n');
    $query->addField('n', 'nid');

    $query->leftJoin('node__body', 'body', 'body.entity_id = n.nid');
    $query->condition('body_value', '%' . $db->escapeLike('data:image/') . '%', 'LIKE');

    $results = $query->execute()->fetchAll();

    return $results;

  }

After the opening curly parenthesis for a function/method there must not be empty lines; the same is true before the closing curly parenthesis for a function/method.

      foreach ($img as $link) {

        $imgTitle = $link->getAttribute('title');
        $imgSrc = $link->getAttribute('src');

        $linkHref = explode(',', $imgSrc);
        $linkHref = end($linkHref);

        if (base64_decode($linkHref, TRUE)) {
          if (empty($imgTitle)) {
            $imgTitle = $link->getAttribute('src');
          }

          $datas[] = [
            'id' => $node->nid->value,
            'title' => $node->toLink(),
            'descriptionSubstr' => substr($imgTitle, 0, 70),
            'description' => $link->getAttribute('src'),
            'imgTitle' => $link->getAttribute('title'),
          ];
        }

      }

    if (base64_decode($linkHref, TRUE)) {

      $decode = base64_decode($linkHref);

      // Get title.
      $imgTitle = $selected["imgTitle"];

      $date = getdate();
      $date = $date['seconds'] . $date['minutes'] . $date['hours'] . $date['mday'] . $date['mon'] . $date['year'];

      if (empty($imgTitle)) {
        // If image don't have title.
        // Generate title with nid, parts of the base64 and the current date.
        $new_file = $directory . $node->nid->value . '-' . substr($linkHref, 120, 10) . '-' . $date . '.' . $imgType;
      }
      else {
        // If image already have title.
        // The file get the name of the image title.
        $new_file = $directory . $imgTitle . '.' . $imgType;
      }

      $file_exists = FileSystemInterface::EXISTS_REPLACE;
      file_save_data($decode, $new_file, $file_exists);

    }

The first and the last line in a foreach() (or a if()) control structure must not be empty lines.

     $rows[$i] = [

        new FormattableMarkup('<a href="@src" > Display </a>', ['@src' => $data['description']]),
        $data['descriptionSubstr'],
        $data['title'],
        $data["id"],

      ];

When defining an array, there must not be empty lines after the opening parenthesis or before the closing parenthesis.

Hello,
Thanks for your reviews.
I have applied modifications :

1. Change description of class and functions
2. Remove configFactory
3. Add translatable string
4. Change the function getImgDatabse to use entityTypeManager
5. Delete empty lines
6. Others little modification

For the batch. When you say the code is loading all the nodes to process before running the batch. Do you refer to the loading of the function getInfoImage and the loading of the function getImgDatabase ?

Thank you for your replied.

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 Slack → #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 reviewers.

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