php-sap / common

Exceptions and abstract classes containing logic for PHP/SAP that is not specific to the underlying PHP module.
https://php-sap.github.io/
MIT License
0 stars 1 forks source link

Breaking Change: setParams Function in AbstractFunction Enforces Mandatory Parameters, Causes Incompatibility with Optional SAP Parameters #25

Closed ynnoig closed 2 months ago

ynnoig commented 2 months ago

The recent implementation of the setParams function in the \phpsap\classes\AbstractFunction class introduces a breaking change that causes compatibility issues when handling optional SAP parameters.

Problem:

The current setParams function mandates that all keys defined in getAllowedKeys() must be present in the provided $params array. If any key is missing, the function throws an InvalidArgumentException. This behavior does not align with SAP's design, where input parameters can be optional.

Affected Code:

/**
 * Extract all expected SAP remote function call parameters from the given array
 * and set them.
 * @param array<string, null|bool|int|float|string|array<int|string, mixed>> $params An array of SAP remote function call parameters.
 * @return $this
 * @throws ConnectionFailedException
 * @throws IncompleteConfigException
 * @throws InvalidArgumentException
 * @throws UnknownFunctionException
 */
public function setParams(array $params): IFunction
{
    foreach ($this->getAllowedKeys() as $key) {
        if (!array_key_exists($key, $params)) {
            throw new InvalidArgumentException(sprintf(
                '%s is missing parameter key %s!',
                static::class,
                $key
            ));
        }
        $this->set($key, $params[$key]);
    }
    return $this;
}

Previous Implementation:

The previous implementation in the setMultiple function did not enforce this strict requirement and allowed for optional parameters, which is more aligned with SAP's behavior.

/**
 * This method extracts only allowed keys from the given array. This way it
 * behaves differently than the set() method. This method will never throw an
 * exception because of an invalid key.
 * @param array $data Associative array of keys and values to set.
 * @throws InvalidArgumentException
 */
protected function setMultiple(array $data)
{
    foreach ($this->getAllowedKeys() as $key) {
        if (array_key_exists($key, $data)) {
            $this->setValue($key, $data[$key]);
        }
    }
}

Impact:

This change disrupts the expected behavior where input parameters to an SAP remote function call can be optional. The strict enforcement of mandatory parameters leads to unexpected exceptions, breaking the functionality of existing implementations that rely on optional parameters.

Suggestion:

Consider reverting to the previous behavior or modifying the setParams method to accommodate optional parameters, ensuring compatibility with SAP's design.

If desired, I can create a pull request to address this issue. Please let me know.

Thank u in advance!

ynnoig commented 2 months ago

The check to ensure all mandatory input elements are provided is already implemented in the \phpsap\saprfc\Traits\ParamTrait::getInputParams function, which also properly handles optional parameters.