API page: https://api.drupal.org/api/drupal/core%21modules%21user%21src%21UserData...

The $return variable is useless initialized three times (and each assignment span over three lines). 

  // If $module, $uid, and $name were passed, return the value.
  if (isset($name) && isset($uid)) {
    $result = $result->fetchAllAssoc('uid');
    if (isset($result[$uid])) {
      return $result[$uid]->serialized ? unserialize($result[$uid]->value) : $result[$uid]->value;
    }
    return NULL;
  }
  elseif (isset($uid)) {
    $return = [
      
    ];
    foreach ($result as $record) {
      $return[$record->name] = $record->serialized ? unserialize($record->value) : $record->value;
    }
    return $return;
  }
  elseif (isset($name)) {
    $return = [
      
    ];
    foreach ($result as $record) {
      $return[$record->uid] = $record->serialized ? unserialize($record->value) : $record->value;
    }
    return $return;
  }
  else {
    $return = [
      
    ];
    foreach ($result as $record) {
      $return[$record->uid][$record->name] = $record->serialized ? unserialize($record->value) : $record->value;
    }
    return $return;
  }

Since those are three different branches of the same if statement, it is enough to just initialize the variable once.
Also, those aren't a if() elseif() else() statement because they always return to the caller (if the condition is true). They are independent if() statements, and one can be removed.

  $return = [];
  // If $module, $uid, and $name were passed, return the value.
  if (isset($name) && isset($uid)) {
    $result = $result->fetchAllAssoc('uid');
    if (isset($result[$uid])) {
      return $result[$uid]->serialized ? unserialize($result[$uid]->value) : $result[$uid]->value;
    }
    return NULL;
  }
  if (isset($uid)) {
    foreach ($result as $record) {
      $return[$record->name] = $record->serialized ? unserialize($record->value) : $record->value;
    }
    return $return;
  }
  if (isset($name)) {
    foreach ($result as $record) {
      $return[$record->uid] = $record->serialized ? unserialize($record->value) : $record->value;
    }
    return $return;
  }
  foreach ($result as $record) {
    $return[$record->uid][$record->name] = $record->serialized ? unserialize($record->value) : $record->value;
  }
  return $return;
CommentFileSizeAuthor
#6 initialize-variable-once-2924683-6.patch1.43 KBavpaderno
#5 initialize-variable-once-2924683-5.patch1.46 KBavpaderno

Comments

kiamlaluno created an issue. See original summary.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

I would also add the comments found in branch 8.4.x.

  // If $module, $uid, and $name were passed, return the value.
  if (isset($name) && isset($uid)) {
    $result = $result->fetchAllAssoc('uid');
    if (isset($result[$uid])) {
      return $result[$uid]->serialized ? unserialize($result[$uid]->value) : $result[$uid]->value;
    }
    return NULL;
  }
  $return = [];
  // If $module and $uid were passed, return data keyed by name.
  if (isset($uid)) {
    foreach ($result as $record) {
      $return[$record->name] = $record->serialized ? unserialize($record->value) : $record->value;
    }
    return $return;
  }
  // If $module and $name were passed, return data keyed by uid.
  if (isset($name)) {
    foreach ($result as $record) {
      $return[$record->uid] = $record->serialized ? unserialize($record->value) : $record->value;
    }
    return $return;
  }
  foreach ($result as $record) {
    $return[$record->uid][$record->name] = $record->serialized ? unserialize($record->value) : $record->value;
  }
  return $return;
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Also the last loop should be commented.

  // If $module, $uid, and $name were passed, return the value.
  if (isset($name) && isset($uid)) {
    $result = $result->fetchAllAssoc('uid');
    if (isset($result[$uid])) {
      return $result[$uid]->serialized ? unserialize($result[$uid]->value) : $result[$uid]->value;
    }
    return NULL;
  }
  $return = [];
  // If $module and $uid were passed, return data keyed by name.
  if (isset($uid)) {
    foreach ($result as $record) {
      $return[$record->name] = $record->serialized ? unserialize($record->value) : $record->value;
    }
    return $return;
  }
  // If $module and $name were passed, return data keyed by uid.
  if (isset($name)) {
    foreach ($result as $record) {
      $return[$record->uid] = $record->serialized ? unserialize($record->value) : $record->value;
    }
    return $return;
  }
  // Otherwise, return data keyed by user ID and data name. 
  foreach ($result as $record) {
    $return[$record->uid][$record->name] = $record->serialized ? unserialize($record->value) : $record->value;
  }
  return $return;
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

See #2886365: Comments for the UserData::get() code are unclear and wrongly use "was" instead of "were", which fixed the already existing comments in branch 8.3.x.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new initialize-variable-once-2924683-5.patch1.46 KB

Apparently, spanning the variable initialization on three lines is an artifact of the code showing the source code on Drupal.org API. Also, the shown code doesn't seem to be up-to-date, since I can see more comments in the code taken from the repository.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
StatusFileSize
new initialize-variable-once-2924683-6.patch1.43 KB

There are two lines that are reported to be wrongly indented.

rang501’s picture

rang501 commented
Status: Needs review » Reviewed & tested by the community

Hi!

This seems good to me and the tests are not failing, so I would mark it as RTBC.

cilefen’s picture

cilefen commented
Title: Variable is useless initialized three times » Variable is uselessly initialized three times
Category: Bug report » Task

That’s assuming the class is completely covered by tests of course. I do not think this is a bug. It’s more of a task.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

It seems the only test is for the view handler that retrieves a value using the user.data service (the UserDataTest class). I suppose the test would fail, if the patch introduced a bug in the service.

As for adding tests to that service, I think there is already an issue opened for that, even if it has not been further developed as other issues for the same service.

  • xjm committed d8dc1a0 on 8.5.x 
    Issue #2924683 by kiamlaluno: Variable is uselessly initialized three...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

Nice cleanup. I almost said that this was changing the logic by changing those elseif to if, but there are early returns so it doesn't matter. (Methods that do half a dozen different things depending on combinations of inputs are kind of a code smell anyway... it'd be much better to have dedicated getters with each kind of inputs. I think that's material for a followup, to add explicit methods and deprecate this one.)

If UserDataTest is the only test, that's definitely not complete coverage for all the paths in this method. So we do need to be careful of introducing bugs. I decided not to block this cleanup on adding tests, but a followup might be to use a data provider and a unit test to test all the possible combinations of inputs for this method.

This patch is most easily reviewed with git diff --color-words. I read the word diff over carefully and confirmed that this cleanup doesn't change the logic of anything.

Committed and pushed to 8.5.x. Thanks! (I chose not to backport it for now even though it's an internal cleanup, given the lack of test coverage and all. This kind of refactoring is risky because it tends to introduce bugs, so no need to risk the next patch release.)

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

The issue for adding tests for the user.data service is #2373573: UserData needs testing, where chx also suggests the get() method is not the best. (His words are currently the get method looks like something the cat dragged in.)

Since that issue is still using the return-different-things-get() module, I would rather open a different issue for splitting the get() method. After that is done, we can implement a straight test for the value returned from the user.data service.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

I opened #2925525: Split UserdataInterface::get() in different methods as follow-up.

Status: Fixed » Closed (fixed)

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