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

[BUGFIX] Use TYPO3 http client #372

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Mabahe
Copy link
Contributor

@Mabahe Mabahe commented Oct 22, 2024

Since all $GLOBALS['TYPO3_CONF_VARS']['HTTP'] options should be respected we can simple use the built in client. Also 'proxy' is respected in this case, so we don't need a extra setting for this.

Resolves: #370
Relates: #331

@NarkNiro sorry for removing your proxy fix again. Would be great, if you could varify this change also works for you.

Since all $GLOBALS['TYPO3_CONF_VARS']['HTTP'] options should be
respected we can simple use the built in client. Also 'proxy'
is respected in this case, so we don't need a extra setting for this.

Resolves: web-vision#370
Relates: web-vision#331
@NarkNiro NarkNiro requested a review from sbuerk October 30, 2024 11:01
Copy link
Contributor

@sbuerk sbuerk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See other comment, in general okay. DI would be awesome, but as already commented we will do that later with thinking about some streamlinen in the same run for the next version.

@NarkNiro Feel free to merge.

if ($proxyUrl !== null) {
$options[TranslatorOptions::PROXY] = $proxyUrl;
}
$options[TranslatorOptions::HTTP_CLIENT] = GeneralUtility::makeInstance(GuzzleClientFactory::class)->getClient();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In TYPO3 v11 GuzzleClientFactory::getClient() is static, that changed in v12. However, it is safe to call it the non-static way in both cases on a class instance [1].

It would be properly better to get the factory or injected, but I would take that for now this way and streamline that with the next version (dropping v11 support).
And we can think with time if we want directly a client instance injected instead of the factory along with some streamlining. Thus, postponing making this DI aware right now.

Setting a full client is supported by the DeepL PHP API [2], which make this a doable replacement.

Would make sense to add additional tests to the line with the DI change when mockedfactory can be used.

[1] See https://3v4l.org/a8RI2
[2] See https://github.com/DeepLcom/deepl-php?tab=readme-ov-file#custom-http-client

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.

[BUG] Target Language (ISO Code) [deeplTargetLanguage] is empty
2 participants