Compatibility issue after update to OC10.11

[lukasmatusiewicz]

The new "guzzle/guzzle" version doesn't allow adding arrays into the "body" request option anymore. This dependency update causes a compatibility issue for the apps, that implement the IClientService and are sending the requests using the body option.
E.g.: privacyidea/privacyidea-owncloud-app#93.

Using the "form_params" option fixes this problem for us, but I found no information about it in OC documentation.

This is an example of the property-built POST request placed in the PHPDoc comment in "oc-core/lib/public/Http/Client/IClient.php" :

'body' => [
    'field' => 'abc',
    'other_field' => '123',
    'file_name' => fopen('/path/to/file', 'r'),
    ],

This way to forward the arrays doesn't work anymore (from v10.11).

[phil-davis]

Hmmm, yes. guzzlehttp/guzzle/src/Client.php public function requestAsync() has:

        if (\is_array($body)) {
            throw $this->invalidBody();
        }

Looking at other places like guzzlehttp/psr7/src/ServerRequest.php the body needs to be passed as string|resource|StreamInterface|null - that is, something that is a "string/stream of bytes" that can be directly sent in the request.

[phil-davis]

I can see these existing bits of code that I suspect will not work:
https://github.com/owncloud/core/blob/master/apps/federation/lib/BackgroundJob/RequestSharedSecret.php#L143

			$result = $this->httpClient->post(
				$target . $this->endPoint,
				[
					'body' => [
						'url' => $source,
						'token' => $token,
					],
					'timeout' => 3,
					'connect_timeout' => 3,
				]
			);

https://github.com/owncloud/core/blob/master/lib/private/HTTPHelper.php#L107

			$response = $client->post(
				$url,
				[
					'body' => $fields,
					'connect_timeout' => 10,
				]
			);

https://github.com/owncloud/market/blob/master/lib/HttpService.php#L304

	private function httpPost($path, $options) {
		$ca = $this->config->getSystemValue('marketplace.ca', null);
		if ($ca !== null) {
			$options = \array_merge(
				[
					'verify' => $ca
				],
				$options
			);
		}
		$client = $this->httpClientService->newClient();

		try {
			$response = $client->post($path, $options);
		} catch (TransferException $e) {
			throw new AppManagerException(
				$this->l10n->t(
					'No marketplace connection: %s',
					[$e->getMessage()]
				),
				0,
				$e
			);
		}

		$result = $this->httpPost($url, [
			'body' => [
				'loginToken' => $loginToken,
				'codeVerifier' => $codeVerifier
			]
		]);

All of those try to send an array as body. IMO use of those will fail.

I wonder why nothing in the test suites have noticed a problem?

@jvillafanez or @DeepDiver1975 or ? Who can/should take a look.

[phil-davis]

And as well as double-checking and fixing the code, the PHP doc needs to be fixed up, and something put in developer release notes for people who use this stuff in their oC10 apps.

[jvillafanez]

Other than fixing our stuff, I don't think we need to do anything else, or am I missing something? I mean, whether "body" or "form_params" is used, depends on whoever is calling the function. We can't perform an automatic transformation.
We'll have to track all the places where we're using the http client and check if we can change the "body" to "form_params" or there is another thing we need to do.

[phil-davis]

Yes, I think that we need to:

• correct the example comments that are with the various functions
• look at the code fragments that I found Compatibility issue after update to OC10.11 #40649 (comment) and sort out what breaks in practice, add unit tests for those cases, refactor the code to fix it (or if the code is really not used anywhere by anyone, then deleting might be a way of refactoring!)

[lukasmatusiewicz]

I've tested using "form_params" with the privacyIDEA app, what helped to walk around the problem in v10.11, but sadly in v10.8 this solution isn't working and causes this error:
No method can handle the form_params config key
Maybe you should concider downgrading the "guzzle" and plan this upgrade to v11.0 with a proper dependency note.

[jvillafanez]

I'm not sure if we want to downgrade guzzle. I'll leave the final decision to @DeepDiver1975

For your particular case, assuming we still want to keep an updated guzzle version, the easiest solution could be to require ownCloud version 10.11 in your app's info.xml file. 10.10 still has guzzle 5.3. Another option could be to detect the ownCloud version and prepare the parameters accordingly.

I'll make a PR fixing our stuff in order to be prepared.

[jvillafanez]

It seems the only "real" bug is in the background job. The rest seems to be unused / deprecated / dead code.

[phil-davis]

Should be fixed by #40652
I wrote a changelog, see PR #40654
@lukasmatusiewicz do these things fix the issue?

[lukasmatusiewicz]

Thank you @phil-davis , I've changed our code too, to adjust the request option type on the fly. So, there is no burning need for us to fix it.

[phil-davis]

The related changes will be released in the next 10.12 release.