[2.0.x] Media placeholder

Thank you for applying! Reviewers will review the project files, describing what needs to be changed.

Please read Review process for security advisory coverage: What to expect → for more details and Security advisory coverage application checklist → to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smoother review.

To reviewers: Please read How to review security advisory coverage applications → , What to cover in an application review → , and Drupal.org security advisory coverage application workflow → .

While this application is open, only the user who opened the application can make commits to the project used for the application.

Reviewers only describe what needs to be changed; they don't provide patches to fix what reported in a review.

Fix phpcs issues.

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml media_placeholder/

FILE: media_placeholder/media_placeholder.module
--------------------------------------------------------------------------------
FOUND 9 ERRORS AFFECTING 9 LINES
--------------------------------------------------------------------------------
  14 | ERROR | [x] Opening brace should be on the same line as the declaration
  31 | ERROR | [x] Opening brace should be on the same line as the declaration
  45 | ERROR | [x] Opening brace should be on the same line as the declaration
  56 | ERROR | [x] Opening brace should be on the same line as the declaration
  66 | ERROR | [x] Expected newline after closing brace
  91 | ERROR | [x] Opening brace should be on the same line as the declaration
 100 | ERROR | [x] Expected newline after closing brace
 103 | ERROR | [x] Expected newline after closing brace
 118 | ERROR | [x] Opening brace should be on the same line as the declaration
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 9 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: media_placeholder/media_placeholder.routing.yml
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 7 | WARNING | The administration page callback should probably use "administer site configuration" - which implies the user can change something - rather than "access
   |         | administration pages" which is about viewing but not changing configurations.
--------------------------------------------------------------------------------

Sorry I tagged the wrong branch initially. I updated the phpcs issues and readme already in branch 2.0.x

FILE: media_placeholder.routing.yml

_permission: 'access administration pages'

The administration page callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations.

Hi Vishal, thanks for the feedback. I've changed the permission.

Rest looks fine to me.

Let’s wait for other reviewers to take a look and if everything goes fine, you will get the role.

Look good for me.

Hi all,
Would like to hear what to do next? Or are we still waiting for more reviewers?

Targeting branch 2.0.x have found 2 PHPCS warnings. Please fix the below.

FILE: /media_placeholder/README.md
-----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-----------------------------------------------------------------------
 3 | WARNING | Line exceeds 80 characters; contains 84 characters
 4 | WARNING | Line exceeds 80 characters; contains 84 characters
-----------------------------------------------------------------------

Is there anything else that must be changed? We do not block an application for that, at this stage of the review.

@apaderno Apart from #13 other changes have seemed to be addressed in previous comments.

src/Form/ConfigForm.php

    $form['provider_picsum']['info'] = [
      '#type' => 'item',
      '#markup' => $this->t('Service provided by <a href="https://picsum.photos/">picsum.photos</a>.<br /> We use <strong>Static Random Images</strong> with a "static" seed based on image config variables.'),
    ];

URLs are placed in a translatable string using placeholders. Translators cannot translate URLs and it is better to leave them out of strings to translate.

media_placeholder.module

    $text = urlencode($config->get('img_text'));
    if (empty($text)) {
      $text = urlencode("image not found!");
    }
    return "https://via.placeholder.com/{$width}x{$height}.png?text={$text}";

If the content of $text is shown in the user interface, as text or text in an image, it needs to be translated, either when the value used comes from the configuration object or the default "Image not found!" value is used. For translating configuration values, see Translating configuration → .

I've addressed the issues previously mentioned. Looking forward to your feedback or to be able to opt into security advisory coverage.

Fix me

Non-blocking comments

I am changing priority as per Issue priorities → .

This thread has been idle, in the needs work state with no activity for several months. Therefore, I am assuming that you are no longer pursuing this application, and I marked it as Closed (won't fix).

If this is incorrect, and you are still pursuing this application, then please feel free to re-open it and set the issue status to Needs work or Needs review, depending on the current status of your code.