[1.0.x] Migrate Process HTML

Thank you for applying!

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 smother review.

The important notes are the following.

• If you have not done it yet, you should run phpcs --standard=Drupal,DrupalPractice on the project, which alone fixes most of what reviewers would report.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status won't be changed by this application and no other user will be able to opt projects into security advisory policy.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.

To the 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 → .
Reviewer's task is describing what needs to be changed, not providing patches to fix what reported in a review.

Please fix the below PHPCS issues.

FILE: /migrate_process_html/migrate_process_html.info.yml
-----------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------
 1 | WARNING | Remove "version" from the info file, it will be added by drupal.org packaging automatically
-----------------------------------------------------------------------------------------------------------


FILE: /migrate_process_html/README.md
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
 51 | WARNING | Line exceeds 80 characters; contains 107 characters
--------------------------------------------------------------------------


FILE: /migrate_process_html/src/Plugin/migrate/process/MigrateProcessHtml.php
---------------------------------------------------------------------------------------------------------------------------------------------
FOUND 40 ERRORS AND 2 WARNINGS AFFECTING 32 LINES
---------------------------------------------------------------------------------------------------------------------------------------------
  12 | ERROR   | [x] Whitespace found at end of line
  15 | ERROR   | [x] Whitespace found at end of line
  51 | ERROR   | [x] Whitespace found at end of line
  57 | ERROR   | [x] Whitespace found at end of line
  61 | ERROR   | [x] Data types in @var tags need to be fully namespaced
  68 | ERROR   | [ ] Description for the @return value is missing
  76 | ERROR   | [x] Whitespace found at end of line
  80 | ERROR   | [x] Expected 1 space after asterisk; 3 found
  82 | ERROR   | [x] Expected 1 space after asterisk; 3 found
  84 | ERROR   | [x] Expected 1 space after asterisk; 3 found
  86 | ERROR   | [x] Expected 1 space after asterisk; 3 found
  86 | ERROR   | [x] Data types in @param tags need to be fully namespaced
  91 | ERROR   | [x] Whitespace found at end of line
 103 | ERROR   | [x] Whitespace found at end of line
 105 | ERROR   | [x] Inline comments must start with a capital letter
 105 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
 112 | ERROR   | [ ] The array declaration extends to column 150 (the limit is 80). The array content should be split up over multiple lines
 112 | ERROR   | [x] There should be no white space before a closing "]"
 114 | ERROR   | [x] Whitespace found at end of line
 120 | ERROR   | [x] Expected newline after closing brace
 123 | ERROR   | [x] Whitespace found at end of line
 124 | ERROR   | [x] Inline comments must start with a capital letter
 124 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
 124 | ERROR   | [x] Whitespace found at end of line
 126 | ERROR   | [x] Whitespace found at end of line
 127 | ERROR   | [x] Inline comments must start with a capital letter
 128 | WARNING | [x] There must be no blank line following an inline comment
 128 | WARNING | [ ] There must be no blank line following an inline comment
 131 | ERROR   | [x] Inline comments must start with a capital letter
 131 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
 132 | ERROR   | [x] Whitespace found at end of line
 134 | ERROR   | [x] Whitespace found at end of line
 135 | ERROR   | [x] Inline comments must start with a capital letter
 135 | ERROR   | [x] Whitespace found at end of line
 140 | ERROR   | [x] Inline comments must start with a capital letter
 140 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
 148 | ERROR   | [ ] The array declaration extends to column 152 (the limit is 80). The array content should be split up over multiple lines
 148 | ERROR   | [x] There should be no white space before a closing "]"
 156 | ERROR   | [x] Expected newline after closing brace
 160 | ERROR   | [x] Expected newline after closing brace
 163 | ERROR   | [x] Whitespace found at end of line
 165 | ERROR   | [x] Whitespace found at end of line
---------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 38 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------------------------------------------------


FILE: /migrate_process_html/src/Form/SettingsForm.php
------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
------------------------------------------------------------------------------------------------------
 35 | WARNING | Avoid backslash escaping in translatable strings when possible, use "" quotes instead
 38 | WARNING | Avoid backslash escaping in translatable strings when possible, use "" quotes instead
 39 | WARNING | Avoid backslash escaping in translatable strings when possible, use "" quotes instead
------------------------------------------------------------------------------------------------------


FILE: /migrate_process_html/tests/src/Functional/LoadTest.php
--------------------------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
--------------------------------------------------------------------------------------------------
 26 | ERROR | [x] Whitespace found at end of line
 28 | ERROR | [x] Whitespace found at end of line
 30 | ERROR | [x] Doc comment short description must end with a full stop
 33 | ERROR | [x] Whitespace found at end of line
--------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------------

Thankyou.

I have run phpcs and hopefully fixed the PHPCS warnings as posted previously.

Hopefully should be ready to review again

You might have missed these two. Please fix

FILE: /migrate_process_html/src/Plugin/migrate/process/MigrateProcessHtml.php
----------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
----------------------------------------------------------------------------------------------------------------------------------
 106 | WARNING | Line exceeds 80 characters; contains 81 characters
 157 | ERROR   | The array declaration extends to column 151 (the limit is 80). The array content should be split up over
     |         | multiple lines
----------------------------------------------------------------------------------------------------------------------------------

Is there anything that should be changed? Code lines are allowed to exceed 80 characters, as per Drupal coding standards. Code simpler to understand is preferred over code that not exceed 80 characters.

Apart from #6 other changes have seemed to be addressed in previous comments.

Hi @vinaymahale

I have reworked a little so hopefully even #6 should be covered now.

I am changing priority as per Review process for security advisory coverage: What to expect / Application Review Timelines → .

To the reviewers

Please set the priority to Normal after a review.

1. master is a wrong branch name, as branch names end with the literal .x. That branch needs to be removed.

2. Fix phpcs issue.

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

FILE: migrate_process_html/src/Plugin/migrate/process/MigrateProcessHtml.php
-------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------
 6 | ERROR | [x] Use statements should be sorted alphabetically. The first wrong one is Drupal\migrate\MigrateExecutableInterface.
-------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------

I was getting a lot of deprecated notices and not able to reproduce when running phpcs. After updating phpcs from 3.5.8 to 3.7.2 I was able to get the same.

I have now :

1. deleted master branch
2. fixed phpcs warning with phpcbf

Thanks

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.

No issues found
Changing status to RTBC!

• What follows is a quick review of the project; it doesn't mean to be complete
• For each point, the review usually shows some lines that should be fixed (except in the case the point is about the full content of a file); it doesn't show all the lines that need to be changed for the same reason
• 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/Plugin/migrate/process/MigrateProcessHtml.php

The class implements ContainerFactoryPluginInterface, but it does not use create() for all its dependencies.

          $error_url_code_message = $this->t('Failed to get URL @url "@error". @code', [
            '@url' => $ahref,
            '@error' => $e->getMessage(),
            '@code' => $e->getCode(),
          ]);

          \Drupal::logger('migrate_process_html')->notice($error_url_code_message);

Logged messages are not translatable strings. Translatable strings are used only when shown in the user interface; the logger stores the message in the database.
Exceptions are also logged with watchdog_exception().

/src/Form/SettingsForm.php

For what I can see, none of the migration plugins use a setting form. The settings for a migration plugin are entered in the migration file and the plugins access their settings with $this->configuration.

As a side note, the plural of redirect is redirects; redirect's is the possessive of redirect.

Hi @apaderno

Thanks for the quick review.

I have attempted to address most of the issues you raised. Namely:

1. I have tried to inject a psr-3 type specific logging channel as documented here → . D8 - injecting a specific channel. tbh I don't really know if this is working as expected so might be a good case for writing a test to check if this logs as expected when catching and logging any exceptions.
2. Removed use of t() on Logged messages. Now just uses a string.
3. Removed use of settings form and replaced with $this->configuration.
4. Fixed punctuation.

So, I think the main outstanding issue is the test coverage here which I admit could be better. That said my test coverage does seem in line with other contrib modules listed here → .

That said, it would be good to have a test to test the logging. I have had a quick look at the contrib migrate_plus module and still need to look into creating some Units tests similar to say migrate_plus/tests/src/Unit/process/StrReplaceTest.php so probably need to figure out how that works first.

I would expect Drupal core has already tests for exception logging. I do not think it is necessary to have tests for that, except in the case the module logs exception in a particular way.

As a side note, $this->logger->notice() is able to use placeholders. For example, the following code correctly use $this->logger->notice().

$this->logger->notice('Failed to get URL @url "@error". @code', [
  '@url' => $value,
  '@error' => $e->getMessage(),
  '@code' => $e->getCode(),
]);

The following code is not correct.

$error = $e->getMessage();
$code = $e->getCode();
$error_url_code_message = "Failed to get first URL $value, $error, $code";
        
// Logs a notice if we get http error response.
$this->logger->notice($error_url_code_message);

Thanks for the feedback apaderno.

Yes, I do not think I am logging in a particular way.

As suggested I have adjusted $this->logger->notice() to use placeholder's.

Thanks again for you comments about my incorrect usage of JavaScript redirect's.

Not sure why I added an apostrophe to redirects?! I was thinking of a JavaScript type redirect, that causes a redirect or multiple redirects, i.e. a
single redirect or multiple redirections caused by the Javascript redirect.

Thanks again for clarifying.

The test class is not testing the module. Drupal core already has tests for the front page; there is no need for contributed module to add the same test.

Thanks apaderno

I tend to agree with you regarding the value of that test.

I have now has a chance to remove that test and also add a couple that are testing this plugin/module.

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.

Assumption is the mother of all evil

Thank you for your contribution!
I updated your account so you can now opt into security advisory coverage for any project you created and every project you will create.

These are some recommended readings to help you with maintainership:

• Dries → ' post on Responsible maintainers
• Maintainership →
• Git version control system →
• Issue procedures and etiquette →
• Maintaining and responding to issues for a project →
• Release naming conventions → .

You can find more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !

Thank you 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 the dedicated reviewers as well.

Many thanks.

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