The attached patch makes a large number of minor changes to make the Code Review module happy.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wizonesolutions’s picture

Status: Needs review » Needs work

I support these changes wholeheartedly. THANK YOU SO MUCH FOR MAKING THEM. I've been bugged by some of the old code in this module but have just never taken the time to clean it up. Props of the highest kind.

A few notes to myself on things to tweak before I commit the patch. No action from you required.

+++ b/fillpdf.admin.incundefined
@@ -174,7 +174,7 @@ function fillpdf_forms_admin_validate($form, &$form_state) {
+  //from includes/file.inc, line 634, but can't use that function because file not an object yet

Note to self: Space after double slash.

+++ b/fillpdf.moduleundefined
@@ -214,12 +216,14 @@ function fillpdf_parse_uri() {
+    drupal_set_message(t('Fillpdf Form ID required to print a PDF.'), 'warning');

Note to self: Fix spelling here.

wizonesolutions’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Needs work » Patch (to be ported)

Committed to 7.x-1.x.

We should do this for the 6.x version as well. It's probably even worse (Coder Upgrade would have corrected some of the deficiencies when porting it to 7).

Liam Morland’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.02 KB

Here are a few additional D7 changes.

Liam Morland’s picture

Here are the changes for D6.

wizonesolutions’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Assigned: Unassigned » wizonesolutions

The 7.x patch has issues. Identifying what they are and will produce a differential meant for the latest codebase to fix it.

wizonesolutions’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Assigned: wizonesolutions » Unassigned
FileSize
721 bytes

OK, this is the differential patch to revert drupal_strlen to strlen. This is what caused the issue.

Back to the focus on D6.

Liam Morland’s picture

FileSize
840 bytes

Thanks. The attached patch adds a comment to explain why strlen() is used instead of drupal_strlen().

wizonesolutions’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Needs review » Needs work

Thanks. I'm just nit-picking:

+++ b/fillpdf.moduleundefined
@@ -425,7 +425,8 @@ function fillpdf_merge_pdf($fid, $nids = NULL, $webform_arr = NULL, $sample = NU
+  drupal_add_http_header('Content-Length', strlen($data)); // This must be strlen(), not drupal_strlen()

Put this comment above the line since it runs onto two lines.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
766 bytes

Updated patch attached. I started the comment on the same line so that it would show up in Code Review next to the message telling people to consider changing the strlen() to drupal_strlen(). I'm fine with it either way.

wizonesolutions’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev

Yeah, I know what you mean. But if they're already running it through Coder (the 20% case), we'll favor code style (the 80% case) and just hope they look at the surrounding lines as well. Thanks!

Committed to 7.x-1.x.

Changing back to 6.x again for the backport.

wizonesolutions’s picture

#1430390: Coding standards: remove whitespace breaks this patch. Is it possible to re-roll?

wizonesolutions’s picture

Status: Needs review » Needs work
Liam Morland’s picture

Can that patch be backed-out then? Making a whitespace patch is dead easy. The original patch in this issue is much more work. Same issue with #1355018: Settings page behavior is confusing.

I don't use D6 and I don't really have time to justify working on it.

AlexBorsody’s picture

I will test this and if it works commit it with the whitespace fixed.

  • Commit 47f74a0 on 7.x-1.x, 7.x-2.x, 7.x-2.x-tests1, 7.x-1.x-ubercartbackporttest authored by Liam Morland, committed by wizonesolutions:
    Issue #1392922: Additional code style fixes.
    
    
  • Commit 5dcf6f4 on 7.x-1.x, 7.x-2.x, 7.x-2.x-tests1, 7.x-1.x-ubercartbackporttest by wizonesolutions:
    Issue #1392922: Revert to strlen().
    
    drupal_strlen() appears to be...
  • Commit 72e12f7 on 7.x-1.x, 7.x-2.x, 7.x-2.x-tests1, 7.x-1.x-ubercartbackporttest authored by Liam Morland, committed by wizonesolutions:
    Issue #1392922: Correct Coder Review-reported code issues.
    
    
  • Commit a036f30 on 7.x-1.x, 7.x-2.x, 7.x-2.x-tests1, 7.x-1.x-ubercartbackporttest authored by Liam Morland, committed by wizonesolutions:
    Issue #1392922: Explain why strlen() is used.
    
    

Liam Morland’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Issue summary: View changes
Status: Needs work » Closed (fixed)

Drupal 6 is no longer supported.