Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JsonCollector: Introduce GetOptionalHeaders() and GetCurlOptions() methods #44

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

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
core/jsoncollector.class.inc.php Outdated Show resolved Hide resolved
core/jsoncollector.class.inc.php Outdated Show resolved Hide resolved
core/jsoncollector.class.inc.php Outdated Show resolved Hide resolved
core/jsoncollector.class.inc.php Outdated Show resolved Hide resolved
core/jsoncollector.class.inc.php Outdated Show resolved Hide resolved
core/jsoncollector.class.inc.php Outdated Show resolved Hide resolved
core/jsoncollector.class.inc.php Show resolved Hide resolved
@jbostoen jbostoen marked this pull request as draft January 16, 2024 12:21
core/jsoncollector.class.inc.php Outdated Show resolved Hide resolved
core/jsoncollector.class.inc.php Outdated Show resolved Hide resolved
core/jsoncollector.class.inc.php Outdated Show resolved Hide resolved
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
Member

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
Member

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

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 technical review
4 participants