ext/soap: Fix SOAP classmap validation for integer keys

[LamentXU123]

Previously, SOAP only rejected packed arrays, which still allowed sparse integer-keyed arrays and mixed string integer-keyed arrays to be accepted.

Since classmap is expected to be an associative mapping, arrays containing integer keys are now rejected consistently.

[devnexen]

could you add the following test please ?

--TEST--
SoapClient must not read a packed "classmap" array as a hash
--EXTENSIONS--
soap
--FILE--
<?php

class LocalClient extends SoapClient
{
    public function __doRequest(string $request, string $location, string $action, int $version, bool $oneWay = false, ?string $uriParserClass = null): ?string
    {
        echo $request, PHP_EOL;

        return <<<XML
        <?xml version="1.0" encoding="UTF-8"?>
        <SOAP-ENV:Envelope xmlns:SOAP-ENV="http://schemas.xmlsoap.org/soap/envelope/">
          <SOAP-ENV:Body><ns1:doItResponse xmlns:ns1="urn:test"/></SOAP-ENV:Body>
        </SOAP-ENV:Envelope>
        XML;
    }
}

$client = new LocalClient(null, [
    'location' => 'http://example.com/',
    'uri' => 'urn:test',
    'classmap' => ['SomeType' => 'stdClass'],
]);

$property = new ReflectionProperty(SoapClient::class, '_classmap');
$property->setValue($client, ['stdClass', 'stdClass']);

$client->__soapCall('doIt', [new stdClass()]);

echo 'done', PHP_EOL;

?>
--EXPECTF--
%A
done

[LamentXU123]

This is indeed a bug when SoapClient::_classmap can be reflected as a packed array :)

[devnexen]

I would like to see the following in classmap_invalid_keys.phpt test :

    'sparse numeric' => [100 => 'stdClass'],
    'numeric string' => ['1' => 'stdClass'],

those new tests

--TEST--
SoapClient and SoapServer report an invalid "classmap" option differently
--EXTENSIONS--
soap
--FILE--
<?php

$classmap = ['type' => 'stdClass', 1 => 'stdClass'];

try {
    new SoapClient(null, [
        'location' => 'http://example.com/',
        'uri' => 'urn:test',
        'classmap' => $classmap,
    ]);
} catch (Throwable $e) {
    echo 'SoapClient: ', $e::class, ': ', $e->getMessage(), PHP_EOL;
}

try {
    new SoapServer(null, [
        'uri' => 'urn:test',
        'classmap' => $classmap,
    ]);
} catch (Throwable $e) {
    echo 'SoapServer: ', $e::class, ': ', $e->getMessage(), PHP_EOL;
}

?>
--EXPECT--
SoapClient: SoapFault: SoapClient::__construct(): 'classmap' option must be an associative array
SoapServer: ValueError: SoapServer::__construct(): Argument #2 ($options) "classmap" option must be an associative array

--TEST--
SoapClient reports an invalid "classmap" option as a fatal error when exceptions are disabled
--EXTENSIONS--
soap
--FILE--
<?php

new SoapClient(null, [
    'location' => 'http://example.com/',
    'uri' => 'urn:test',
    'exceptions' => false,
    'classmap' => ['type' => 'stdClass', 1 => 'stdClass'],
]);

echo 'not reached', PHP_EOL;

?>
--EXPECTF--
Fatal error: SoapClient::__construct(): 'classmap' option must be an associative array in %s on line %d

[devnexen]

name a bit misleading since you check only the keys

[devnexen]

since we re in the "TODO mindset", what about having a ValueError instead (to match the server side) ?

[LamentXU123]

I don't have strong opinions on this tho. But sure, this makes the behavior of SoapClient and SoapServer the same.

[LamentXU123]

If we throw error here, the code won't goes to ZVAL_RES(). So we need to manually delete this.
the context is created with an additional ref:

  context = php_stream_context_from_zval(tmp, 1);
  Z_ADDREF_P(tmp);

[devnexen]

yes but in this case we can avoid this gymnastic by checking this before instantiating the stream context.

[devnexen]

if we have a packed array with one element but we unset it, should not trigger exception right ? might be a nice test case.

[devnexen]

I think global $soap_version needs to be initialised since this property has no default.

[devnexen]

it might be preferable to throw if this is not an array