Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The attached patch makes a large number of minor changes to make the Code Review module happy.
Comment | File | Size | Author |
---|---|---|---|
#9 | fillpdf_strlen.patch | 766 bytes | Liam Morland |
#7 | fillpdf_strlen.patch | 840 bytes | Liam Morland |
#6 | fillpdf_1392922_strlen_revert.patch | 721 bytes | wizonesolutions |
#4 | fillpdf_coding_standards-D6.patch | 71.84 KB | Liam Morland |
#3 | fillpdf_coding_standards-D7_2.patch | 1.02 KB | Liam Morland |
Comments
Comment #1
wizonesolutionsI 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.
Note to self: Space after double slash.
Note to self: Fix spelling here.
Comment #2
wizonesolutionsCommitted 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).
Comment #3
Liam MorlandHere are a few additional D7 changes.
Comment #4
Liam MorlandHere are the changes for D6.
Comment #5
wizonesolutionsThe 7.x patch has issues. Identifying what they are and will produce a differential meant for the latest codebase to fix it.
Comment #6
wizonesolutionsOK, this is the differential patch to revert
drupal_strlen
tostrlen
. This is what caused the issue.Back to the focus on D6.
Comment #7
Liam MorlandThanks. The attached patch adds a comment to explain why strlen() is used instead of drupal_strlen().
Comment #8
wizonesolutionsThanks. I'm just nit-picking:
Put this comment above the line since it runs onto two lines.
Comment #9
Liam MorlandUpdated 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.
Comment #10
wizonesolutionsYeah, 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.
Comment #11
wizonesolutions#1430390: Coding standards: remove whitespace breaks this patch. Is it possible to re-roll?
Comment #12
wizonesolutionsComment #13
Liam MorlandCan 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.
Comment #14
AlexBorsody CreditAttribution: AlexBorsody commentedI will test this and if it works commit it with the whitespace fixed.
Comment #18
Liam MorlandDrupal 6 is no longer supported.