GitLab CI reports PHP_CodeSniffer errors/warnings which should be fixed.

CommentFileSizeAuthor
#15 redirect_node-phpcs_result.png19.87 KBsilvi.addweb
#15 redirect_node-applied_patch.png55.27 KBsilvi.addweb
#11 interdiff-5-11.txt723 bytesnitin_lama
#11 3355409-11.patch1.52 KBnitin_lama
#5 interdiff_2-5.txt711 bytesakram khan
#5 3355409-5.patch1.53 KBakram khan
#3 with patch.png144.43 KBkenyoowen
#3 without patch.png158.66 KBkenyoowen
#2 3355409-2.patch1.54 KBsamitk

Issue fork redirect_node-3355409

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

samit.310@gmail.com created an issue. See original summary.

samitk’s picture

samitk commented
Assigned: samitk » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3355409-2.patch1.54 KB

Above errors/warnings has been fixed.

kenyoowen’s picture

kenyoowen commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new without patch.png158.66 KB
new with patch.png144.43 KB

Hi samit.310@gmail.com

I applied the patch #2 to the “Redirect Nodes” module against Version 8.x-1..x-dev coding-standard-fixes.patch and confirmed that the errors is resolved. Please see the screenshots attached.

For your review.
Thank you.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Reviewed & tested by the community » Needs work 
+    // Calls _redirect_node_replace_node_url_walk_array() recursively on all
+    // menu items and their children. If the url for a menu item is rewritten,
+    // then a js script is attached that will inject edit links on each of the
+    // menu items.
     if (isset($build['content']['#items'])) {

In comments used inside functions or method, verbs use the imperative.
Also, instead of js script, it should be JavaScript code.

akram khan’s picture

akram khan
Primary language English
Location Cuttack, Odisha
commented
Status: Needs work » Needs review
StatusFileSize
new 3355409-5.patch1.53 KB
new interdiff_2-5.txt711 bytes

Addressed #4

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work 
+    // Calls _redirect_node_replace_node_url_walk_array() recursively on all
+    // menu items and their children. If the url for a menu item is rewritten,
+    // attach JavaScript code that will inject edit links on each of the
+    // menu items.

Calls is the indicative of call, not its imperative.
url is misspelled, since that is an acronym.

Preethy_ray made their first commit to this issue’s fork.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Also, If a menu item URL is rewritten is probably better.

pray_12’s picture

pray_12 commented
Issue summary: View changes
nitin_lama’s picture

nitin_lama
he/him
Location India
commented
Assigned: Unassigned » nitin_lama
nitin_lama’s picture

nitin_lama
he/him
Location India
commented
StatusFileSize
new 3355409-11.patch1.52 KB
new interdiff-5-11.txt723 bytes

Providing updated patch for 8.x-1.x-dev branch. Addressing comment #6, #8.

nitin_lama opened merge request !4

nitin_lama’s picture

nitin_lama
he/him
Location India
commented
Status: Needs work » Needs review

Addressing changes in the MR. Please review.

nitin_lama’s picture

nitin_lama
he/him
Location India
commented
Assigned: nitin_lama » Unassigned
silvi.addweb’s picture

silvi.addweb commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new redirect_node-applied_patch.png55.27 KB
new redirect_node-phpcs_result.png19.87 KB

The above MR !4 looks good, no phpcs error/warning was found.
Thanks

apaderno opened merge request !6

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue summary: View changes

richgerdes made their first commit to this issue’s fork.

richgerdes’s picture

richgerdes
he/him
Primary language English
Location New Jersey, USA
commented
Status: Reviewed & tested by the community » Fixed

Thanks for the hard work and patches.

The patch for the ci job is great. Will be nice to validate the code. I've addressed the additional items reported by the ci jobs here as well.

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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