Skip to content

#4469: Pass Drupal Http Client to Harvester#4474

Draft
stefan-korn wants to merge 1 commit intoGetDKAN:2.xfrom
stefan-korn:4469-harvester-proxy-fix
Draft

#4469: Pass Drupal Http Client to Harvester#4474
stefan-korn wants to merge 1 commit intoGetDKAN:2.xfrom
stefan-korn:4469-harvester-proxy-fix

Conversation

@stefan-korn
Copy link
Contributor

Fixes #4469

Describe your changes

Just passing the Drupal HTTP Client to the Harvester. If you have suggestion, like do it with dependency injection in the harvester service, let me know.

QA Steps

  • Add manual QA steps in checklist format for a reviewer to perform. Be as specific as possible, provide examples if appropriate.

Checklist before requesting review

If any of these are left unchecked, please provide an explanation

  • I have updated or added tests to cover my code
  • I have updated or added documentation

@stefan-korn
Copy link
Contributor Author

okay, codeclimate says it all ;-)

\Drupal calls should be avoided in classes, use dependency injection instead

Will take a look into this.

@dafeder
Copy link
Member

dafeder commented Apr 25, 2025

@stefan-korn yes we would need to add http client as a constructor for the service. Codeclimate will surely complain that it's too many arguments but that one we can ignore; I think the solution there is just to split this into two services or something because as it is now they're all needed.

Also BTW I don't know why we have "getHarvester" and "getDkanHarvesterInstance" they seem totally redundant. I'll probably squash those into one method at some point.

@dafeder
Copy link
Member

dafeder commented Jun 23, 2025

@stefan-korn do you plan to fix this? Thanks

@dafeder dafeder marked this pull request as draft August 27, 2025 17:13
@dafeder
Copy link
Member

dafeder commented Oct 23, 2025

@stefan-korn bump :)

Make sure to rebase this if you pick it back up, as you need to switch to the new qlty tooling

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Harvester does not use proxy settings

2 participants