This is part of this meta issue: Make flag module pass coder review

In this issue, we aim just to fix the coding standards and not any doc block changes.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cs_shadow’s picture

Status: Active » Needs review
FileSize
96.46 KB

This patch most of the coding standard related issues. Most of the remaining issues are related to missing doc blocks, invalid/missing type hinting or missing scope identifiers which will be fixed as separate issues.

joachim’s picture

Youch. That's going to take a while to go through!

joachim’s picture

+++ b/flag.module
@@ -2199,11 +2222,11 @@ function flag_get_entity_flags($entity_type, $entity_id, $flag_name = NULL) {
@@ -2224,7 +2247,7 @@ function flag_create_link($flag_name, $entity_id, $variables = array()) {

@@ -2224,7 +2247,7 @@ function flag_create_link($flag_name, $entity_id, $variables = array()) {
 /**
  * Trim a flag to a certain size.
  *
- * @param $fid
+ * @param $flag
  *   The flag object.

That belongs in a bug report.

joachim’s picture

  1. +++ b/flag.module
    @@ -1555,11 +1565,11 @@ function template_preprocess_flag(&$variables) {
    -  // Some typing shotcuts:
    +  // Some typing shotcuts.
    

    That colon should stay.

  2. +++ b/flag.module
    @@ -1689,7 +1700,7 @@ function _flag_link_type_descriptions() {
     // ---------------------------------------------------------------------------
    -// Non-Views public API
    +// Non-Views public API.
    

    I don't think there's any convention for these separator markers -- in fact I think core frowns on them. I prefer without a full stop as it's a title.

  3. +++ b/flag.tokens.inc
    @@ -123,6 +123,7 @@ function flag_tokens($type, $tokens, array $data = array(), array $options = arr
             case 'name':
               $replacements[$original] = $sanitize ? check_plain($flag->name) : $flag->name;
               break;
    +
             case 'title':
               $replacements[$original] = $sanitize ? check_plain($flag->get_title()) : $flag->get_title();
    

    I'm not convinced that a blank line after each switch case improves readability.

  4. +++ b/flag_bookmark/includes/flag_bookmark.views_default.inc
    @@ -244,7 +244,7 @@ function flag_bookmark_views_default_views() {
     
    -  $view = new view;
    +  $view = new view();
       $view->name = 'flag_' . $flag->name;
    

    That's part of a direct paste from Views export. That shouldn't be changed unless the Views export changes.

  5. +++ b/includes/flag.export.inc
    @@ -236,7 +237,12 @@ function flag_update_page($flag) {
    -  drupal_set_message(t('The flag %name is currently using the Flag API version @version, which is not compatible with the current version of Flag. You can upgrade this flag by pasting the below code into <em>@module_flag_default_flags()</em> function in the @module.module file.', array('%name' => $flag->name, '@version' => $flag->api_version, '@module' => $flag->module)), 'warning');
    +  drupal_set_message(t('The flag %name is currently using the Flag API version @version, which is not compatible with the current version of Flag. You can upgrade this flag by pasting the below code into <em>@module_flag_default_flags()</em> function in the @module.module file.', array(
    +    '%name' => $flag->name,
    +    '@version' => $flag->api_version,
    +    '@module' => $flag->module,
    +  )),
    +    'warning');
    

    Weird formatting here!

  6. +++ b/tests/flag.test
    @@ -712,7 +724,7 @@ class FlagAccessLinkTestCase extends FlagTestCaseBase {
    -    $flag_user = $this->drupalCreateUser(array('flag test_flag',));
    +    $flag_user = $this->drupalCreateUser(array('flag test_flag'));
    

    This is old code but I can see a reason for keeping this terminal comma.

  7. +++ b/tests/flag.test
    @@ -1202,8 +1221,6 @@ class FlagHookInvocationsTestCase extends FlagTestCaseBase {
         $hook_data_variable = variable_get('flag_hook_test_hook_tracking', array());
    -    //debug($hook_data_variable['hook_entity_presave']);
    -    //debug($hook_data_variable['hook_entity_insert']);
     
    

    Don't take my test debug statements away!

cs_shadow’s picture

@joachim, I'll update the patch but before that I wanted to clarify few things:

Reg #2.2, Do you want the separation markers to be removed?

Reg #2.3, Its just that Core does it this way, so shouldn't follow those standards?

Reg #2.7, Do we really want our debug statements in production code?

I agree with the other things you pointed out and will fix them soon.

joachim’s picture

> I'll update the patch

Don't do that yet -- I'm halfway through converting it to a sequential patch.

> Reg #2.2, Do you want the separation markers to be removed?

I quite like them :)

> Reg #2.3, Its just that Core does it this way, so shouldn't follow those standards?

You're right, the coding standards have a space after each case in the example.

> Reg #2.7, Do we really want our debug statements in production code?

In tests, I'd say yes! They save time when updating or adding to tests!

cs_shadow’s picture

Oh, that's good. I was just about to start to work on the patch. Would have been a lot of duplicate work.

joachim’s picture

Here's most of your changes, but grouped into smaller commits in a sequential patch.

To apply this, use 'git apply' in a feature branch: it'll create a string of commits.

One thing I've removed is the changes to documentation indentation, this sort of thing:

+++ b/flag.api.php
@@ -47,7 +47,7 @@ function hook_flag_type_info() {
  * @param $definitions
- *  An array of flag definitions returned by hook_flag_type_info().
+ *   An array of flag definitions returned by hook_flag_type_info().
  */

I know the docs standards say to use indentation like that, but I hate it!!! My text editor puts me on odd-numbered columns when I hit tab, and this standard means the first character of documentation can't be reached by tabbing. Not going to happen in any of my modules, sorry (unless someone can help me configure my text editor to handle this!!!).

cs_shadow’s picture

Changes are much more clear with this sequential patch. Looks good. Do you think we can commit this so that i can start with the documentation issues?

joachim’s picture

Status: Needs review » Fixed

Committed! Thanks for working on this.

git commit -m "Issue #2245601 by cs_shadow, joachim: Fixed coding standards errors." --author="cs_shadow "

BTW, the way to make sequential patches like that is:

- make a local branch for your work (which is a good idea anyway)
- make your commits
- do 'git format-patch --stdout' to get a sequential diff

  • Commit 3779d0c on 7.x-3.x authored by cs_shadow, committed by joachim:
    Issue #2245601 by cs_shadow, joachim: Fixed coding standards errors.
    
cs_shadow’s picture

Thanks @joachim. #10 was really helpful.

Status: Fixed » Closed (fixed)

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