Hi,

I took the initiative to run the computed field module, which we are using through a code reviewer.
I used the http://pareview.sh service.

Just a few minor issues have been found.

Regards,
Lachezar

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lachezar.valchev’s picture

Status: Needs work » Needs review
FileSize
16.44 KB

Attached the patch and mark it as Needs review.

naveenvalecha’s picture

Issue tags: -PAReview

Removing unused tag.
The above patch has used the coding standard.AFAIK on d.org there is no tag for coding standard.

lachezar.valchev’s picture

Issue tags: -Core review

Hi,

Removed unnecessary tag.

Regards,
Lachezar

naveenvalecha’s picture

  1. +++ b/computed_field.info
    @@ -3,5 +3,3 @@ description = Defines a field type that allows values to be "computed" via PHP c
    \ No newline at end of file
    

    Add new line at EOF

  2. +++ b/computed_field.module
    @@ -306,36 +327,49 @@ function computed_field_field_formatter_info() {
    +    // to avoid undefined index errors in typical PHP display code.
    

    First to should be "To"

lachezar.valchev’s picture

Status: Needs review » Needs work
lachezar.valchev’s picture

Version: 7.x-1.0 » 7.x-1.x-dev
Status: Needs work » Needs review
FileSize
11.1 KB

Hi,

Thanks for the notes. I ran the latest 7.x-1.x against the latest Coder (Drupal coding standards and Drupal best practices) and prepared updated patch for the issues.

All of previous code style issues apart from the "First to should be To" were already there, but there were bunch of other issues, reflected in the new one.

Regards,
Lachezar

naveenvalecha’s picture

Status: Needs review » Needs work

Thanks! for the updated patch.It is near RTBC , only few more tweaks are needed.

  1. +++ b/computed_field.module
    @@ -77,7 +78,7 @@ function computed_field_field_settings_form($field, $instance, $has_data) {
    +    '#description' => t('<p>The variables available to your code include: <code>@fields

    . To set the value of the field, set @entity_field. For multi-value computed fields continue with @entity_field_multi. Here\'s a simple example which sets the computed field\'s value to the value of the sum of the number fields (@field_a and @field_b) in a node entity:

    !example

    Alternately, this code can be supplied by your own custom function named: @compute_func(&$entity_field, $entity_type, $entity, $field, $instance, $langcode, $items).

    ', array(

    Remove the html tags

    out from t() function.Usage of html inside should always be avoided.Please append and prepend p tag around t() function.

  2. +++ b/computed_field.module
    @@ -432,14 +452,6 @@ function _computed_field_compute_value($entity_type, $entity, $field, $instance,
    -  // Setup a variable with the entity language if available.
    -  if (isset($entity->language)) {
    -    $entity_lang = $entity->language;
    -  }
    -  else {
    -    $entity_lang = LANGUAGE_NONE;
    -  }
    -
    

    Why this change ?

lachezar.valchev’s picture

Hi @naveenvalecha,

Thanks for the review and continuously help.

I will start with 1. and do the fixes as per your comment.

In regards to 2.: I have removed that piece of code, because it seems like it has never been really used.
The $entity_lang variable is not used anywhere within the function _computed_field_compute_value and it does not seem to be passed by reference as well.
The initial code with the $entity_lang is introduced in commit 096d268, but haven't been used there as well.

Maybe it was a try to handle translated fields, but really it seems unfinished. If that is the case maybe it should be handled it separate issue. But if the variable is not used I think it is in the coding standards that it should be removed for readability and performance. The latest Coder module that I am using 8.x-2.x, which also supports Drupal 7 complained about it as well.

What do you think?

Regards,
Lachezar