Skip to content

Conversation

jbostoen
Copy link

This PR intends to make it easier to extend the JsonCollector.

Use case: A third-party API which requires Basic Authorization with each POST request.

Basic Authorization can basically be performed in these ways:

  1. Using a cURL option: CURLOPT_USERPWD .
  2. By explicitly adding it in the HTTP headers of the POST request.

Without the PR:

  • cURL option: Now, this would however need to be defined in the generic "params.xxx.xml" ("conf" dir). However, most third-party application data seems to be set in the "params.xxx.xml" file of the "collectors" dir.
  • To set custom headers, the whole Prepare() method would need to be edited.

WIth the PR:

  • cURL options can also be defined in the "collectors" XML file.
  • It's also easy to extend the JsonCollector and add a custom HTTP header. The authentication could be coded instead depending on a separate "user" and "pwd" setting in the collector's XML.

By default, no HTTP headers are set.
By default, cURL options = cURL options as defined in the "conf" XML (as before); and cURL options as defined in the "collectors" XML (these are superseding).

Note: I used the correct spelling of "optional". In the source code, it's consistently written incorrectly with "optionnal". I was considering updating it, but that might cause issues if people already extended classes and if those methods were overridden.

@Molkobain Molkobain added the enhancement New feature or request label Jan 16, 2024
@Molkobain Molkobain self-assigned this Jan 16, 2024
@jbostoen jbostoen marked this pull request as draft January 16, 2024 12:21
jbostoen and others added 3 commits January 17, 2024 13:17
Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>
Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>
Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>
@odain-cbd
Copy link
Contributor

I come after all other feedbacks. everything is ok to me. great job and idea!

NB:
today that part of the code was not designed to easily test it. obviously. so i will not push for tests. we will discuss it apart from this PR. there is much more refactoring to reach that goal...

@jbostoen jbostoen marked this pull request as ready for review January 31, 2024 19:00
@Molkobain
Copy link
Contributor

Technical review:

  • Logic seems ok
  • Guillaume will just check if the refacto can be done on a prent class for a better genericity or if it's meant to be only in JSON collector and children.

@Molkobain
Copy link
Contributor

Note: Maybe we should use \CallItopService::CallItopViaHttp() instead and adjust the factorisation

@Molkobain Molkobain removed their assignment Aug 19, 2024
@jf-cbd jf-cbd self-requested a review October 18, 2024 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Pending Combodo update
Development

Successfully merging this pull request may close these issues.

4 participants