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.
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;
Comment | File | Size | Author |
---|---|---|---|
#6 | initialize-variable-once-2924683-6.patch | 1.43 KB | apaderno |
#5 | initialize-variable-once-2924683-5.patch | 1.46 KB | apaderno |
Comments
Comment #2
apadernoI would also add the comments found in branch 8.4.x.
Comment #3
apadernoAlso the last loop should be commented.
Comment #4
apadernoSee #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.
Comment #5
apadernoApparently, 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.
Comment #6
apadernoThere are two lines that are reported to be wrongly indented.
Comment #7
rang501 CreditAttribution: rang501 at ADM Interactive commentedHi!
This seems good to me and the tests are not failing, so I would mark it as RTBC.
Comment #8
cilefen CreditAttribution: cilefen at Institute for Advanced Study commentedThat’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.
Comment #9
apadernoIt 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.
Comment #11
xjmNice cleanup. I almost said that this was changing the logic by changing those
elseif
toif
, 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.)
Comment #12
apadernoThe 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 theget
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 theget()
method. After that is done, we can implement a straight test for the value returned from the user.data service.Comment #13
apadernoI opened #2925525: Split UserdataInterface::get() in different methods as follow-up.