Closed (fixed)
Project:
Project Browser
Version:
2.0.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
12 Sep 2022 at 18:34 UTC
Updated:
26 Nov 2024 at 20:09 UTC
Jump to comment: Most recent
Comments
Comment #2
tim.plunkettComment #3
chrisfromredfinComment #4
chrisfromredfinComment #6
nairb commentedI 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.
Comment #8
nairb commentedI 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.
Comment #9
chrisfromredfinThis looks good and tests are passing. However, I'd like Tim or Fran to give it a review first.
Comment #10
fjgarlin commentedI 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...
Comment #11
chrisfromredfinComment #15
chrisfromredfinI'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.
Comment #16
fjgarlin commentedI 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).
Comment #17
narendrarComment #19
utkarsh_33 commentedRebased this to the latest 2.0.x.There are a couple of issues that @fjgarlin pointed related to
isMaintained and URLso 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 .Comment #20
chrisfromredfinWe 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.
Comment #21
fjgarlin commentedI see field_maintenace_status here: https://www.drupal.org/jsonapi/index/project_modules?filter%5Bmachine_na...
and a query using the filter here
Comment #22
fjgarlin commentedisMaintainedis 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.
Comment #23
utkarsh_33 commentedComment #24
phenaproximaI don't see a problem with this.
Comment #25
phenaproximaComment #26
fjgarlin commentedRTBC+1. The cleanup looks good.
Comment #29
chrisfromredfinThis test seems to fail consistently with the removal of these properties. This needs to be investigated further.
Comment #30
fjgarlin commentedNote 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.
Comment #31
fjgarlin commentedThe lines to remove are in
DrupalDotOrgJsonApi.phpfile and are:Then, when rebasing, you might get a conflict in the following lines:
All you need to do to fix the conflict, is remove those lines entirely, as that is what this issue is intending to do.
Comment #32
narendrarComment #34
chrisfromredfinAt first, with this MR applied, when I go to a module detail page, I get 5 warnings - one for each removed property.
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!