Using sonarqube to review out code we have this "crital" warning:

"Make sure that the referenced value variable is unset after the loop"

in views/plugins/views_wizard/views_ui_base_views_wizard.class.php 

            foreach ($path_fields as &$field) {
              $field['id'] = view::generate_item_id($field['id'], $display_options['default']['fields']);
              $display_options['default']['fields'][$field['id']] = $field;
            }

When a reference is used in a foreach loop instead of using a simple variable, the reference remains assigned and keeps its "value" which is a reference, even after the foreach execution. Most of the time, this is not what the developer is expecting and the reference may be used wrongly in the rest of the code. For this reason, it is recommended to always unset a reference that is used in a foreach to avoid any unexpected side effects.

Noncompliant Code Example

$arr = array(1, 2, 3);
foreach ($arr as &$value) { // Noncompliant; $value is still alive after the loop and references the last item of the array: $arr[2]
    $value = $value * 2;
}
$value = 'x';

Compliant Solution
$arr = array(1, 2, 3);
foreach ($arr as &$value) { // Compliant; there is no risk to use by mistake the content of $value pointing to $arr[2]
    $value = $value * 2;
}
unset($value);
$value = 'x';
CommentFileSizeAuthor
#7 views-n3376114-7.patch8.79 KBdamienmckenna
#7 views-n3376114-7.interdiff.txt8.42 KBdamienmckenna

Issue fork views-3376114

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Carlitus created an issue. See original summary.

Carlitus opened merge request !10

carlitus’s picture

carlitus commented
Status: Active » Needs review
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Version: 7.x-3.29 » 7.x-3.x-dev
Status: Needs review » Needs work

That looks like a useful tiny improvement, thank you for putting it together.

There's a tiny issue with the change - the indenting is incorrect, please re-save the change using two spaces for each indent so that it lines up correctly? Thank you.

carlitus’s picture

carlitus commented

Sorry for the wrong indenting, i think was a nano thing. Now i've made it with phpstorm and it seems right.

carlitus’s picture

carlitus commented
Status: Needs work » Needs review
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Parent issue: » #3347650: Plan for Views 7.x-3.30
StatusFileSize
new views-n3376114-7.interdiff.txt8.42 KB
new views-n3376114-7.patch8.79 KB

Let's extend this to fix similar issues in other parts of the codebase:

grep -r foreach|grep \&
./views_ui.module:  foreach ($plugins['display'] as &$display) {
./plugins/views_wizard/views_ui_base_views_wizard.class.php:            foreach ($path_fields as &$field) {
./plugins/views_wizard/views_ui_base_views_wizard.class.php:    foreach ($display_options as &$options) {
./plugins/views_wizard/views_ui_base_views_wizard.class.php:        foreach ($display_options['default']['fields'] as &$field) {
./plugins/views_plugin_style.inc:      foreach ($classes as &$class) {
./plugins/views_plugin_display.inc:      foreach ($filters as &$filter) {
./includes/admin.inc:    foreach ($new_fields as &$new_field) {
./includes/admin.inc:  foreach ($rows as &$row) {
./views.api.php:    foreach ($query->where as &$condition_group) {
./views.api.php:      foreach ($condition_group['conditions'] as &$condition) {
./views.api.php:  foreach ($commands as &$command) {
./theme/theme.inc:        foreach ($vars['rows'] as $num => &$column_items) {
./modules/comment/views_plugin_row_comment_rss.inc:    foreach ($this->comments as &$comment) {
./modules/field/views_handler_field_field.inc:      foreach ($values as $row_id => &$value) {
./modules/search/views_handler_filter_search.inc:        foreach ($condition_conditions as $key => &$condition) {
./modules/search/views_handler_filter_search.inc:      foreach ($conditions as $key => &$subcondition) {
./modules/search/views_handler_argument_search.inc:        foreach ($condition_conditions as $key => &$condition) {
./modules/system/views_handler_argument_file_fid.inc:    foreach ($titles as &$title) {
./modules/comment.views.inc:    foreach ($build as $cid => &$comment_build) {
./handlers/views_handler_field.inc:    foreach ($classes as &$class) {
./handlers/views_handler_field.inc:    foreach ($classes as &$class) {
./handlers/views_handler_field.inc:    foreach ($classes as &$class) {
./handlers/views_handler_filter_entity_bundle.inc:      foreach ($this->value as &$value) {
./handlers/views_handler_relationship_groupwise_max.inc:    foreach ($conditions as $condition_id => &$condition) {
./help/api-forms.html:    foreach ($this->view->result as $row_id => $row) {
./views.module:  foreach ($conditions as $condition_id => &$condition) {
damienmckenna’s picture

damienmckenna
Location TN, USA
commented

@Carlitus: can you please review the new patch and let me know what you think? Thank you.

carlitus’s picture

carlitus commented

Hi,

I have checked it and I see everything is fine

The only thing is that I have seen some unused instances in the foreach keys, but maybe this is another minor issue.

comment.views.inc

line 708

    foreach ($build as $cid => &$comment_build) {
      if (isset($comment_build['links'])) {
        unset($comment_build['links']);
      }
    }
    unset($comment_build);

theme.inc

        foreach ($vars['rows'] as $num => &$column_items) {
          unset($column_items[$column]);
          unset($vars['header'][$column]);
        }
        unset($column_items);
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Title: Make sure that the referenced value variable is unset after the loop » Unset referenced variables after foreach loops
Status: Needs review » Reviewed & tested by the community

I think it'd be better to do that cleanup separately.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Reviewed & tested by the community » Fixed

Committed.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Related issues: +#3386717: Remove unused variables

Status: Fixed » Closed (fixed)

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