Problem/Motivation

I was testing out the module and I noticed that the BrowserController file can be slightly improved by removing usages of services directly. I am still new to this initiative and not sure why it has been implemented like that in the first place. Adding a patch to resolve this simple issue.

Steps to reproduce

Proposed resolution

Remaining tasks

  • ✅ File an issue about this project
  • ✅ Addition/Change/Update/Fix to this project
  • ☐ Testing to ensure no regression
  • ☐ Automated unit/functional testing coverage
  • ☐ Developer Documentation support on feature change/addition
  • ☐ User Guide Documentation support on feature change/addition
  • ☐ Code review from 1 Drupal core team member
  • ☐ Full testing and approval
  • ☐ Credit contributors
  • ☐ Review with the product owner
  • ☐ Release

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3243019-2.patch3.1 KBgaurav.kapoor

Issue fork project_browser-3243019

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:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gaurav.kapoor created an issue. See original summary.

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at Axelerant for Drupal India Association commented
Status: Active » Needs review
FileSize
3243019-2.patch3.1 KB
gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at Axelerant for Drupal India Association commented
Issue summary: View changes
chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
CreditAttribution: chrisfromredfin at Redfin Solutions, LLC commented

I totally agree that we should be using Dependency Injection instead of reliance on Global Services. I'm not comfortable RTBC'ing this myself, though. Would look to @grasmash or @rlnorthcutt for a review.

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at Axelerant for Drupal India Association commented
Assigned: gaurav.kapoor » Unassigned
chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
CreditAttribution: chrisfromredfin at Redfin Solutions, LLC commented
Status: Needs review » Postponed (maintainer needs more info)

I'm curious if this still applies given the pretty massive rewrite we've gone through.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Postponed (maintainer needs more info) » Needs work

Needs a reroll (may be worth just redoing), but still needs doing

srishti.bankar made their first commit to this issue’s fork.

srishti.bankar opened merge request !40

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review

Following up on the part about $request, before the last commit it would try to request /admin/modules/[object%20Object]/drupal-org-proxy/project instead of http://d8/drupal-org-proxy/project (lol, yes, my d10 site is still called `d8`)

I thought we'd need a whole new test for this, but it actually is a straightforward addition to ProjectBrowserUiTest, effectively undoing/re-fixing #3250003: Unbreak test failures

narendraR’s picture

narendraR
He/Him
Primary language English
Location Jaipur, India
CreditAttribution: narendraR as a volunteer and at Acquia commented
Status: Needs review » Reviewed & tested by the community

Marking as RTBC.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Reviewed & tested by the community » Fixed

Thanks! Merged

Status: Fixed » Closed (fixed)

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