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.
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
Comment | File | Size | Author |
---|---|---|---|
#6 | computed_field-code_review-2324857-6.patch | 11.1 KB | lachezar.valchev |
#1 | computed_field-code_review-2324857-1.patch | 16.44 KB | lachezar.valchev |
Comments
Comment #1
lachezar.valchev CreditAttribution: lachezar.valchev commentedAttached the patch and mark it as Needs review.
Comment #2
naveenvalechaRemoving unused tag.
The above patch has used the coding standard.AFAIK on d.org there is no tag for coding standard.
Comment #3
lachezar.valchev CreditAttribution: lachezar.valchev at FFW commentedHi,
Removed unnecessary tag.
Regards,
Lachezar
Comment #4
naveenvalechaAdd new line at EOF
First to should be "To"
Comment #5
lachezar.valchev CreditAttribution: lachezar.valchev at FFW commentedComment #6
lachezar.valchev CreditAttribution: lachezar.valchev at FFW commentedHi,
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
Comment #7
naveenvalechaThanks! for the updated patch.It is near RTBC , only few more tweaks are needed.
. 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.
Why this change ?
Comment #8
lachezar.valchev CreditAttribution: lachezar.valchev at FFW commentedHi @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