As per the Security Advisory,

https://www.drupal.org/security-advisory-policy

this module which is considered currently to be a release candidate can be brought up to standards.

CommentFileSizeAuthor
#7 pareview_fixes-2861362-7.patch4.34 KBmikebrooks

Comments

darrell_ulm created an issue. See original summary.

darrell_ulm’s picture

darrell_ulm commented
Priority: Normal » Major
darrell_ulm’s picture

darrell_ulm commented

Also for module users, it can be decided to Opt-In in the project edit screen.
It states:

Once you opt-in your project, you may not opt-out. Only the Security Team will be able to change this.

Here is the security advisory coverage: https://www.drupal.org/security-advisory-policy

Have been working more in Apache Spark and other projects recently as a heads up.

mikebrooks’s picture

mikebrooks commented

Hi Darrell,

I am not security vetted yet, though I have an application in the issue queue for another project.

Prerequisite to Security Advisory Coverage is a stable release, i.e. not dev, alpha, beta, or rc.

Given the lack of bugs, I see no reason why we can't change the branch to 7.x-1.0.

Can you make the release update?

mikebrooks’s picture

mikebrooks commented

Please ignore my prior post... just ran PAReview. Lots of issues (see below).

I can work on the issues when I have some free time. They look pretty straight forward.

Review of the 7.x-1.x branch (commit 1f3862d):

  • README.md or README.txt is missing, see the guidelines for in-project documentation.
  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
  • DrupalPractice has found some issues with your code, but could be false positives.
    
FILE: /root/repos/pareviewsh/pareview_temp/inc/ip_path_access_admin.inc
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------
  35 | WARNING | Unused variable $sql.
 248 | WARNING | Form error messages are user facing text and must run
     |         | through t() for translation
--------------------------------------------------------------------------


FILE: /root/repos/pareviewsh/pareview_temp/ip_path_access.module
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 153 | WARNING | Unused variable $page_title.
 253 | WARNING | Unused variable $key.
----------------------------------------------------------------------

Time: 40ms; Memory: 6Mb
  • No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.


FILE: /root/repos/pareviewsh/pareview_temp/inc/ip_path_access_admin.inc
--------------------------------------------------------------------------
FOUND 21 ERRORS AFFECTING 16 LINES
--------------------------------------------------------------------------
  73 | ERROR | [x] Expected 1 blank line after function; 2 found
  87 | ERROR | [x] Expected 1 blank line after function; 2 found
 190 | ERROR | [x] Expected 1 blank line after function; 2 found
 203 | ERROR | [x] Expected 1 blank line after function; 2 found
 224 | ERROR | [x] Space before opening parenthesis of function call
     |       |     prohibited
 225 | ERROR | [x] Space before opening parenthesis of function call
     |       |     prohibited
 225 | ERROR | [x] Space before opening parenthesis of function call
     |       |     prohibited
 226 | ERROR | [x] Space before opening parenthesis of function call
     |       |     prohibited
 226 | ERROR | [x] Space before opening parenthesis of function call
     |       |     prohibited
 246 | ERROR | [x] Expected 1 space after IF keyword; 0 found
 246 | ERROR | [x] Expected 1 space after closing parenthesis; found 0
 251 | ERROR | [x] Expected 1 blank line after function; 2 found
 275 | ERROR | [ ] If the line declaring an array spans longer than 80
     |       |     characters, each element should be broken into its own
     |       |     line
 277 | ERROR | [x] Expected 1 space after IF keyword; 0 found
 277 | ERROR | [x] Expected 1 space after closing parenthesis; found 0
 281 | ERROR | [ ] If the line declaring an array spans longer than 80
     |       |     characters, each element should be broken into its own
     |       |     line
 284 | ERROR | [x] Expected newline after closing brace
 284 | ERROR | [x] Expected 1 space after ELSE keyword; 0 found
 295 | ERROR | [x] Expected 1 blank line after function; 2 found
 314 | ERROR | [x] Expected 1 blank line after function; 2 found
 322 | ERROR | [x] Object operator not indented correctly; expected 4
     |       |     spaces but found 6
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 19 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------


FILE: /root/repos/pareviewsh/pareview_temp/ip_path_access.module
--------------------------------------------------------------------------
FOUND 21 ERRORS AND 1 WARNING AFFECTING 18 LINES
--------------------------------------------------------------------------
  28 | ERROR   | [x] Expected 1 blank line after function; 2 found
  45 | ERROR   | [x] Expected 1 blank line after function; 2 found
  67 | ERROR   | [x] Expected 1 blank line after function; 2 found
 105 | ERROR   | [x] Expected 1 blank line after function; 2 found
 150 | ERROR   | [x] Inline comments must end in full-stops, exclamation
     |         |     marks, colons, question marks, or closing
     |         |     parentheses
 166 | ERROR   | [x] Inline comments must end in full-stops, exclamation
     |         |     marks, colons, question marks, or closing
     |         |     parentheses
 167 | ERROR   | [x] Expected 1 space after IF keyword; 0 found
 167 | ERROR   | [x] Expected 1 space after closing parenthesis; found 0
 168 | ERROR   | [x] Expected 1 space after IF keyword; 0 found
 168 | ERROR   | [x] Expected 1 space after closing parenthesis; found 0
 169 | WARNING | [ ] Only string literals should be passed to t() where
     |         |     possible
 171 | ERROR   | [x] Expected newline after closing brace
 171 | ERROR   | [x] Expected 1 space after ELSE keyword; 0 found
 179 | ERROR   | [x] Expected 1 blank line after function; 2 found
 198 | ERROR   | [x] Expected 1 blank line after function; 2 found
 226 | ERROR   | [x] Expected 1 blank line after function; 2 found
 250 | ERROR   | [x] Inline comments must end in full-stops, exclamation
     |         |     marks, colons, question marks, or closing
     |         |     parentheses
 259 | ERROR   | [x] Expected 1 space after FOREACH keyword; 0 found
 263 | ERROR   | [x] Expected 1 space between double arrow and
     |         |     "$curr_path"; 2 found
 263 | ERROR   | [x] Expected 1 space after "=>"; 2 found
 267 | ERROR   | [x] Expected 1 space after "+="; 2 found
 286 | ERROR   | [x] There should be no white space after an opening "("
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 21 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

Time: 111ms; Memory: 8Mb
darrell_ulm’s picture

darrell_ulm commented

Yes, good idea running PAReview!
They will want to have about everything clean before we apply for the security coverage.
Surprised there were some unused variables in there, but totally possible.

Yes on

Given the lack of bugs, I see no reason why we can't change the branch to 7.x-1.0.

once it runs through clean, and has the same functionality, which it should with the above fixes.

Then we can go stable and submit.

Thank you again.

mikebrooks’s picture

mikebrooks commented
Status: Active » Needs review
StatusFileSize
new pareview_fixes-2861362-7.patch4.34 KB

I am attaching a patch that should resolve the problems raised by PAReview.sh. The coder module indicates all problems are resolved in my local dev environment. I have conducted basic testing of the module. Another set of eyes would be helpful to confirm the update.

If there is no review within 48 hours, I'll commit the changes to the dev branch, unless someone thinks I should give it more time.

darrell_ulm’s picture

darrell_ulm commented

Reviewed, this looks good, anyone who has access can commit when ready, thank you!

mikebrooks’s picture

mikebrooks commented

Thanks Darrell,

I'll commit to the Dev branch tonight. I don't believe that I have rights to create a release, however. I can ask Nishad, or you can take care of it if you have time.

Cheers - Mike

darrell_ulm’s picture

darrell_ulm commented

Thanks @mikebrooks , I should be able to make the 7.x-1.0 release.

  • mikebrooks committed c9b88d0 on 7.x-1.x 
    Issue #2861362 by mikebrooks: PAReview fixes
mikebrooks’s picture

mikebrooks commented
Status: Needs review » Fixed

Patch committed to the 7.x-1.x branch.

mikebrooks’s picture

mikebrooks commented
Assigned: Unassigned » mikebrooks
Status: Fixed » Needs work

Apparently, PAReview is more particular than the coder module. Some more issues to correct.

  • mikebrooks committed 175b30f on 7.x-1.x 
    Issue #2861362 by mikebrooks: PAReview fixes, part 2

  • mikebrooks committed a7ed97b on 7.x-1.x 
    Issue #2861362 by mikebrooks: PAReview fixes, part 3

  • mikebrooks committed a7bb586 on 7.x-1.x 
    Issue #2861362 by mikebrooks: PAReview fixes, part 4

  • mikebrooks committed b4c172a on 7.x-1.x 
    Issue #2861362 by mikebrooks: PAReview fixes, part 5
mikebrooks’s picture

mikebrooks commented
Assigned: mikebrooks » darrell_ulm
Status: Needs work » Needs review

Hi Darrell,

This is as good as it is going to get right now. Feel free to create a stable release.

Review of the 7.x-1.x branch (commit b4c172a):

    Coder Sniffer has found some issues with your code (please check the Drupal coding standards).

    FILE: /root/repos/pareviewsh/pareview_temp/ip_path_access.module
    --------------------------------------------------------------------------
    FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
    --------------------------------------------------------------------------
    167 | WARNING | Only string literals should be passed to t() where
    | | possible
    --------------------------------------------------------------------------

    Time: 139ms; Memory: 8Mb
    DrupalPractice has found some issues with your code, but could be false positives.

    FILE: /root/repos/pareviewsh/pareview_temp/ip_path_access.module
    ----------------------------------------------------------------------
    FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
    ----------------------------------------------------------------------
    250 | WARNING | Unused variable $key.
    ----------------------------------------------------------------------

    Time: 67ms; Memory: 6Mb
    No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.
darrell_ulm’s picture

darrell_ulm commented

OK, the release is made and live, and opt-ed into the policy. Noticed that in the time the policy came out, there was a 60% usage drop, perhaps that was the reason as there were no other reports in the issue queue.

darrell_ulm’s picture

darrell_ulm commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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