FivestarItem drupal-check issues

Created on 22 July 2021, over 3 years ago
Updated 12 October 2023, about 1 year ago

Problem/Motivation

Drupal-check reported some issues in FivestarItem.php, and manually checking these found some areas for improvement.

1. adding docblock type hints to assist checkers detect Vote entity variables;
2. Split out $vote_rating expression and comment on its call of 'rating' - I feel this is a bug but cannot be sure. Either way it needs improved doc or a fix.
3. Notes in the code which appears to be using a hashed IP address to disambiguate anonymous users, but this is very likely to fail.

Proposed resolution

The attached patch is useful for (1), (but there may be a line number fuzz because the diff is against code that has other unrelated changes).

The comments in the patch for (2) and (3) may be useful to commit but may be addressable inline... that's fine.

🐛 Bug report
Status

Needs work

Version

1.0

Component

Code

Created by

🇬🇧United Kingdom rivimey

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇫🇷France O'Briat Nantes

    I add it to the global issue #3377377 about drupal-check

  • Status changed to Needs work about 1 year ago
  • 🇮🇳India sanjayk

    Reroll the patch

  • Status changed to Needs review about 1 year ago
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.5 + Environment: PHP 7.3 & MySQL 5.7
    last update about 1 year ago
    PHPLint Failed
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.5 + Environment: PHP 7.3 & MySQL 5.7
    last update about 1 year ago
    6 pass
  • 🇮🇳India sanjayk

    Removed PHPLint issue

  • Status changed to Needs work about 1 year ago
  • 🇺🇸United States tr Cascadia

    None of the "TODO" comments should be in there. If they are valid things that need to be addressed in the future, then they should use the proper format @todo so we can find these comments in the future when we look for them. AND if a @todo is added, then there must ALSO be a new issue opened so that this to-do item can be tracked in the issue queue. It makes no sense, and doesn't help this module move forward, just to add code comments about things that look wrong or might need to be explained better. If it needs to be explained better, then add a code comment to explain it better - that would be the proper fix.

    This patch also splits one line and declares a new variable so a third @var comment can be added. That's neither good nor bad, but it's not required by drupal-check.

    The rest of the patch is just the "TODO" comments which aren't helping anything. The comment about ->rating is good - that should be a BUG report handled in a new issue. The comment about the IP address is something that should be discussed as a feature request. Neither of those comments should be in this patch.

    This patch has only two useful lines - the two added @var comments. And in general, if @var comments are going to be added to satisfy drupal-check warnings then they should be added everywhere at once, not just in this one file. We don't need individual patches for each individual file to add @var comments. We don't need to review individual patches for individual files that contain @var comments PLUS a bunch of other unrelated changes. One patch, with all the @var comments for the entire module AND NOTHING ELSE would be simpler and quicker to review. It would also be useful because it adds documentation to the code and makes it easier to understand and maintain and modify in the future.

  • 🇮🇳India sanjayk

    @TR As you mentioned I have created a new issue https://www.drupal.org/project/fivestar/issues/3393492 🐛 Add @todo comment in project Needs work .

Production build 0.71.5 2024