Fix the coding standard problems outlined using the below tool:
http://pareview.sh/pareview/httpgitdrupalorgprojectcaptchagit-8x-1x

CommentFileSizeAuthor
#59 fix-pareview-issues-2798991-59.patch5.29 KBminakshiPh
#56 interdiff-2798991-52-56.txt3.98 KBminakshiPh
#56 fix-pareview-issues-2798991-56.patch3 KBminakshiPh
#52 interdiff-2798991-48-52.txt9.14 KBminakshiPh
#52 fix-pareview-issues-2798991-52.patch7.91 KBminakshiPh
#48 interdiff-2798991-46-48.txt2.68 KBminakshiPh
#48 fix-pareview-issues-2798991-48.patch14.96 KBminakshiPh
#46 fix-pareview-issues-2798991-46.patch15.42 KBminakshiPh
#42 fix-automated-test-issue-42.patch610 bytesminakshiPh
#39 interdiff-2798991-37-39.txt2.06 KBminakshiPh
#39 fix-drupal-coding-std-2798991-39.patch3.82 KBminakshiPh
#37 interdiff-2798991-34-37.txt5.3 KBminakshiPh
#37 fix-drupal-coding-std-2798991-37.patch4.02 KBminakshiPh
#34 interdiff-2798991-19-34.txt2.33 KBshruti1803
#34 2798991-34.patch7.72 KBshruti1803
#29 2798991-28.patch5.24 KBrasikap
#26 2798991-26.patch5.33 KByogeshmpawar
#22 2798991-21.patch4.51 KBkeshavv
#19 2798991-19.patch5.75 KBshruti1803
#6 interdiff-2798991-4-5.txt46.89 KBnaveenvalecha
#5 2798991-5.patch54.24 KBnaveenvalecha
#4 2798991-4.patch13.6 KBnaveenvalecha
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

elachlan created an issue. See original summary.

naveenvalecha’s picture

Status: Active » Postponed

The module is still in alpha state, we'll get them fixed before a beta release. There are still lots of good candidates now to get them in before this. Changing it to postponed for now.

Thanks for filing the issue.

naveenvalecha’s picture

Title: Fix coding standards » Fix coding standards & replace the deprecated methods
naveenvalecha’s picture

Status: Postponed » Needs review
FileSize
13.6 KB

Reopening it as I have closed couple of(~5) issues in favor of this one.
Here's the patch

naveenvalecha’s picture

Here's the Round 2 with more fixes

naveenvalecha’s picture

FileSize
46.89 KB

Here's the interdiff

naveenvalecha’s picture

@elachlan ,
Can we get this fixed as its a huge patch and will be painful later to reroll it.

elachlan’s picture

Is that all of them?

naveenvalecha’s picture

naveenvalecha’s picture

Status: Needs review » Needs work
naveenvalecha’s picture

Issue tags: +Novice

  • elachlan committed f246602 on 8.x-1.x
    Issue #2798991 by naveenvalecha: Fix coding standards...
Sonal.Sangale’s picture

Assigned: Unassigned » Sonal.Sangale

  • elachlan committed ab00601 on 8.x-1.x
    Issue #2798991 by naveenvalecha: Fix coding standards...

  • elachlan committed e95ae27 on 8.x-1.x
    Issue #2798991 by naveenvalecha: Fix coding standards...

  • elachlan committed cae4659 on 8.x-1.x
    Issue #2798991 by naveenvalecha: Fix coding standards...
Sonal.Sangale’s picture

Assigned: Sonal.Sangale » Unassigned
shruti1803’s picture

Status: Needs work » Needs review
FileSize
5.75 KB

@Naveen
Fix coding standard issues. The patch given in #5 is failed to apply.

Status: Needs review » Needs work

The last submitted patch, 19: 2798991-19.patch, failed testing.

  • naveenvalecha committed 334180f on 2798991
    Issue #2798991 by naveenvalecha, shruti1803, elachlan, Sonal.Sangale:...
  • elachlan committed ab00601 on 2798991
    Issue #2798991 by naveenvalecha: Fix coding standards...
  • elachlan committed cae4659 on 2798991
    Issue #2798991 by naveenvalecha: Fix coding standards...
  • elachlan committed e783438 on 2798991 authored by naveenvalecha
    Issue #2798991 by naveenvalecha: Fix coding standards...
  • elachlan committed e95ae27 on 2798991
    Issue #2798991 by naveenvalecha: Fix coding standards...
  • elachlan committed f246602 on 2798991
    Issue #2798991 by naveenvalecha: Fix coding standards...
keshavv’s picture

Here is the patch with code standards

rajeshwari10’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 22: 2798991-21.patch, failed testing.

naveenvalecha’s picture

Issue tags: +Needs reroll
yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
5.33 KB

I have rerolled the patch, Fix coding standards & replace the deprecated methods against 8.x-1.x

Status: Needs review » Needs work

The last submitted patch, 26: 2798991-26.patch, failed testing.

rasikap’s picture

Assigned: Unassigned » rasikap
rasikap’s picture

rasikap’s picture

Assigned: rasikap » Unassigned
rasikap’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 29: 2798991-28.patch, failed testing.

naveenvalecha’s picture

Priority: Normal » Minor
Issue tags: -Needs reroll +Coding standards
shruti1803’s picture

shruti1803’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 34: 2798991-34.patch, failed testing.

minakshiPh’s picture

Status: Needs work » Needs review
FileSize
4.02 KB
5.3 KB

Added the new patch.

Kindly review.
Thanks!

ddrozdik’s picture

Status: Needs review » Needs work

Last submitted patch has some mistakes in comments formatting.
1.

+// '@disable' => $captcha_point->url('disable',
+// ['query' => Drupal::destination()->getAsArray()]),
+// '@enable' => $captcha_point->url('enable',
+// ['query' => Drupal::destination()->getAsArray()]),

What the reason of adding more strings. Better to investigate why those strings are commented and still present in the code.
2.

+ *    The utf8 string which will be used to split in characters.

In this place indent is 4 spaces, but should be 2.
3.

+/*
+TODO: write test for CAPTCHAs on admin pages
+TODO: test for default challenge type
+TODO: test about placement (comment form, node forms, log in form, etc)
+TODO: test if captcha_cron does it work right
+TODO: test custom CAPTCHA validation stuff
+TODO: test if entry on status report (Already X blocked form submissions)
+TODO: test space ignoring validation of image CAPTCHA
+TODO: refactor the 'comment_body[0][value]' stuff.
+ */

Multiline comment formatting is incorrect.

@minakshiPh please take a look on the documentation https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

minakshiPh’s picture

Status: Needs work » Needs review
FileSize
3.82 KB
2.06 KB

Hi @ddrozdik,

Thanks for reviewing my patch !!!

Have made the required changes as per #38

Kindly review.
Thanks!

  • elachlan committed 70c0d1a on 8.x-1.x authored by minakshiPh
    Issue #2798991 by minakshiPh, naveenvalecha, shruti1803, rasikap, keshav...
elachlan’s picture

Status: Needs review » Needs work

There are still a heap of errors in the automated review. Thanks for your work on this.

minakshiPh’s picture

Status: Needs work » Needs review
FileSize
610 bytes

Hi @elachlan,

Thanks for reviewing my patch!

Added new patch to fix automated test issue Automated Testing Issue

Kindly review.
Thanks!

  • elachlan committed 7b7b735 on 8.x-1.x authored by minakshiPh
    Issue #2798991 by minakshiPh, naveenvalecha, shruti1803, rasikap, keshav...
elachlan’s picture

Status: Needs review » Needs work

There are still quite a few outstanding issues in the pareview output. We are very close to having it resolved. Thanks for your work on this.

http://pareview.sh/pareview/httpgitdrupalorgprojectcaptchagit-8x-1x

minakshiPh’s picture

Assigned: Unassigned » minakshiPh
minakshiPh’s picture

Assigned: minakshiPh » Unassigned
Status: Needs work » Needs review
FileSize
15.42 KB

Added new patch to resolve pareview issues as mentioned in #44.

Kindly review.
Thanks!

Status: Needs review » Needs work

The last submitted patch, 46: fix-pareview-issues-2798991-46.patch, failed testing.

minakshiPh’s picture

Status: Needs work » Needs review
FileSize
14.96 KB
2.68 KB

Added patch to resolve pareview issues as mentioned in #44.

Kindly review.
Thanks!

Status: Needs review » Needs work

The last submitted patch, 48: fix-pareview-issues-2798991-48.patch, failed testing.

  • elachlan committed 78b1c09 on 8.x-1.x authored by minakshiPh
    Issue #2798991 by minakshiPh, naveenvalecha, shruti1803, rasikap, keshav...
elachlan’s picture

I've pushed through the documentation changes. We will have to narrow down the issues causing test failure before we push through the rest.

minakshiPh’s picture

Status: Needs work » Needs review
FileSize
7.91 KB
9.14 KB

Hi @elachlan,

Thanks for your helpful comments.

I have made changes in some files and tried to fix the errors.

Kindly review.
Thanks!

Status: Needs review » Needs work

The last submitted patch, 52: fix-pareview-issues-2798991-52.patch, failed testing.

  • elachlan committed 173a3fe on 8.x-1.x authored by minakshiPh
    Issue #2798991 by minakshiPh, naveenvalecha, shruti1803, rasikap, keshav...
elachlan’s picture

I've committed the changes for white space or comments.

That will help make the patch smaller and easier to review.

Keep at it!

minakshiPh’s picture

Status: Needs work » Needs review
FileSize
3 KB
3.98 KB

Added new patch to fix other pareview errors.

Kindly review.
Thanks!

  • elachlan committed f6c89e8 on 8.x-1.x authored by minakshiPh
    Issue #2798991 by minakshiPh, naveenvalecha, shruti1803, rasikap, keshav...
elachlan’s picture

Status: Needs review » Needs work

Still a few outstanding parview errors, but doing a lot better.

minakshiPh’s picture

Status: Needs work » Needs review
FileSize
5.29 KB

Added new patch to fix other pareview errors.

Kindly review.
Thanks!

  • elachlan committed 54bb810 on 8.x-1.x authored by minakshiPh
    Issue #2798991 by minakshiPh, naveenvalecha, shruti1803, rasikap, keshav...

  • elachlan committed 0ef17ff on 8.x-1.x
    Issue #2798991 by minakshiPh, naveenvalecha, shruti1803, rasikap, keshav...
elachlan’s picture

Pareview is looking pretty good. The naming convention stuff I think are a false positive.

ddrozdik’s picture

Status: Needs review » Fixed

Let's mark this issue as fixed, and in the future create a new issue if you want fix coding standards. Now this issue has many patches with different contexts and discussions, and it is difficult to follow it and find something.

Status: Fixed » Closed (fixed)

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