The project_browser.browse route has two route parameters:
source (required) -- the ID of the source plugin we're looking atid (optional) -- the ID of a project, within the given source plugin, that we're examining on its own page"Its own page", you say?
Now that project details are displayed in a modal, it's not at all clear that this route parameter needs to exist. If not, we should remove it outright, along with the ProjectBrowser render element's support for it.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | Remove-the-ability-for-the-project-browser-route-to-take-project-ID.3495318.patch | 1.13 KB | niharika.s |
Show commandsStart 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:
changes
Comments
Comment #2
phenaproximaComment #3
niharika.s commentedComment #4
niharika.s commentedI have created the patch apply changes according to issue .please review it .
Comment #5
phenaproximaThat looks like a good start, but we will need it as a merge request (we're no longer using patches) and I think there is at least one other place where the
#idproperty is used in the ProjectBrowser render element that has to be removed.Comment #7
lavanyatalwar commentedWorking on it.
Comment #9
lavanyatalwar commented@phenaproxima
I have created a MR.
Kindly review :)
Comment #10
shalini_jha commentedPatch is converted to MR but Feedback is not addressed mentioned in #5,
According to #5, there is another place where the #id is being used, which needs to be removed. Upon reviewing the code, I found that in the attachProjectBrowserSettings method, the #id is being attached to Drupal settings. Additionally, in the getDrupalSettings method, the $id parameter is passed and used in a condition (empty($id)) to determine the behaviour of the $package_manager array. Since the $id argument has already been removed from the route, should this parameter and its associated logic also be removed from the getDrupalSettings method?
Comment #11
phenaproximaThanks for asking. We should probably keep the logic from that
ifblock, but remove theifitself. In other words, what's in there should always run.Comment #12
shalini_jha commentedComment #13
shalini_jha commented@phenaproxima Thank you for the review & guidance , I have updated the getDrupalSettings method for #id, removing the id condition while keeping the inner logic unchanged.
Additionally, since the $id parameter has been removed from the method, I have also removed the following doc block comment.
During testing, I noticed that ?id was being appended to the URL for all the tabs. After debugging, I removed the id from the Local task, and it is now working fine. Moving this for your review. Kindly review and let me know if anything else needs to be updated.
Comment #14
phenaproximaThat looks good to me. The test failures do not appear to be related. Thanks!
Comment #17
chrisfromredfinThis feels good to remove cruft from a feature we no longer have! 💪