Problem/Motivation

The \Drupal\project_browser\ProjectBrowser\Project object has several properties which are not used

Proposed resolution

Decide if they should be removed.

Remaining tasks

  • ✅ File an issue about this project
  • ☐ Manual Testing
  • ☐ Code Review
  • ☐ Accessibility Review
  • ☐ Automated tests needed/written?
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:

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

Issue tags: +core-post-mvp
chrisfromredfin’s picture

chrisfromredfin’s picture

Issue tags: +Novice

nairb made their first commit to this issue’s fork.

nairb’s picture

I reviewed the properties marked as unused in the Project file. I found that 'selector_id' is being used but no others are referenced from any svelte files.

nairb’s picture

I removed some parameters from the public function, since they are not needed in Svelte. I left the private functions to set those parameters since they are used when getting the project data from drupal.org, and we'll probably want to have that in case there is a need to display that info.

The Star User Count is never used and is only ever set to a static value. So this could likely be removed completely. Talking with Chris at DrupalCon, it sounds like the star count won't be used, I just wanted to check before stripping it out since I've had more time to understand what is going on.

chrisfromredfin’s picture

Status: Active » Needs review

This looks good and tests are passing. However, I'd like Tim or Fran to give it a review first.

fjgarlin’s picture

Status: Needs review » Needs work

I think that if we are going to remove the exposed properties because they are not used, then we should also remove all the associated methods, like setters and getters.

For example: https://git.drupalcode.org/search?search=setIsActive&nav_source=navbar&p...

chrisfromredfin’s picture

chrisfromredfin changed the visibility of the branch 3309273-audit-the-unused to hidden.

chrisfromredfin’s picture

Status: Needs work » Needs review

I've refactored this since the previous one was so dated, but I think this is better and spreads deeper into the codebase; would love Fran to look at it.

fjgarlin’s picture

Status: Needs review » Needs work

I left some feedback in the MR. If we are cleaning up purely based on what's shown in the svelte templates, then it might be ok, but there are certain properties that are in use one way or another (ie: via filters) or that should be used but might be not (ie: URL).

narendrar’s picture

Version: 1.0.x-dev » 2.0.x-dev

utkarsh_33 made their first commit to this issue’s fork.

utkarsh_33’s picture

Status: Needs work » Needs review

Rebased this to the latest 2.0.x.There are a couple of issues that @fjgarlin pointed related to isMaintained and URL so wanted a consensus on what needs to be done.I'll again mark this NR just to double confirm what are next steps on this and maybe ping in the slack channel .

chrisfromredfin’s picture

Status: Needs review » Needs work

We definitely want to keep URL - we're using that on the detail page now. What I'm amazed by is that getting rid of isMaintained here still lets the "maintained" filter work on the front-end. Why is that?! Do we not need that in the backend object because the front-end doesn't directly use the backend object? Or does the JSON:API endpoint just ignore filters provided that don't matter?

I can't seem to find an example where an unmaintained module shows with manual testing. Can anyone help?

The first order of business here is for someone to digest and understand the interplay between the unused project properties we're removing from the PHP objects, and how those are then used on the frontend. Me, without that knowledge, cannot review this and know if getting rid of these properties "works."

But I can say we need isMaintained and URL on the front-end, for sure.

fjgarlin’s picture

I see field_maintenace_status here: https://www.drupal.org/jsonapi/index/project_modules?filter%5Bmachine_na...
and a query using the filter here

    // 'Actively maintained'.
    '49125cb6-2f35-451b-922d-3042cb1b4391',
    // 'Minimally maintained'
    'de457d5a-2ce3-45b4-b88b-1c54e5e6d0e2',
    // 'Seeking co-maintainer(s)
    'fd8b539f-a5e4-4577-9367-f119a252327b',
fjgarlin’s picture

isMaintained is just a boolean that says whether the maintenance status of a module is within those 3 posted above.
Code: isMaintained: in_array($maintenance_status['id'], self::MAINTAINED_VALUES),

The filters work independently from how that property is calculated.

utkarsh_33’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I don't see a problem with this.

phenaproxima’s picture

Title: Audit the unused Project properties » Remove unused properties from the Project class
fjgarlin’s picture

RTBC+1. The cleanup looks good.

chrisfromredfin changed the visibility of the branch 3309273-audit-unused-v2 to hidden.

chrisfromredfin’s picture

Status: Reviewed & tested by the community » Needs work

This test seems to fail consistently with the removal of these properties. This needs to be investigated further.

fjgarlin’s picture

Note that in issue #3485386: Do not hardcode UUIDs in DrupalJsonApi plugin, we are changing some of the properties that will be removed in this issue, only to avoid a hard dependency on this issue.

If #3485386: Do not hardcode UUIDs in DrupalJsonApi plugin gets merged first, we will need to clean here 3 extra lines that were added. If this one gets merged first, I'll take care of things on the other issue.

fjgarlin’s picture

The lines to remove are in DrupalDotOrgJsonApi.php file and are:

// @todo Remove the next three lines when https://www.drupal.org/project/project_browser/issues/3309273 is merged.
$filter_values = $this->filterValues();
$active_values = $filter_values['active'] ?? [];
$maintained_values = $filter_values['maintained'] ?? [];

Then, when rebasing, you might get a conflict in the following lines:

isMaintained: in_array($maintenance_status['id'], $maintained_values),
...
isActive: in_array($development_status['id'], $active_values),

All you need to do to fix the conflict, is remove those lines entirely, as that is what this issue is intending to do.

narendrar’s picture

Status: Needs work » Needs review

chrisfromredfin’s picture

Status: Needs review » Fixed

At first, with this MR applied, when I go to a module detail page, I get 5 warnings - one for each removed property.

Deprecated function: Creation of dynamic property Drupal\project_browser\ProjectBrowser\Project::$created is deprecated in Drupal\Component\Serialization\PhpSerialize::decode() (line 21 of core/lib/Drupal/Component/Serialization/PhpSerialize.php).

However, I did a fresh install of Drupal (drush si) and it wasn't there - so I'm guessing that was from lingering cache / KeyValueStore stuff from testing other MR's. I think this is good, especially with passing tests!

Status: Fixed » Closed (fixed)

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