commercetools / commercetools-php-sdk

The e-commerce SDK from commercetools for PHP.
https://docs.commercetools.com/sdk/php-sdk#php-sdk-v1
MIT License
43 stars 22 forks source link

fieldDefinitions for most of the flags are missing optional key/flag #704

Closed raju-cimpress closed 2 years ago

raju-cimpress commented 2 years ago

Describe the bug fieldDefinitions() function for most of the resource object is missing self::OPTIONAL => true, due to which calling unsetting any value gives error.

To Reproduce Create an object of Address or Customer class and try to call a function to unset an option field. Example for Address object $address try $address->setCompany() i.e, parameter passed to the function. It throws error argument one is expected as string null passed.

Expected behavior $address->setCompany() should work without passing any parameter.

Screenshots/Code snippet As per the documentation https://docs.commercetools.com/api/types#address, Address class should be.

class Address extends JsonObject
{
    public function fieldDefinitions()
    {
        return [
            'id' => [self::TYPE => 'string', self::OPTIONAL => true],
            'key' => [self::TYPE => 'string', self::OPTIONAL => true],
            'title' => [self::TYPE => 'string', self::OPTIONAL => true],
            'salutation' => [self::TYPE => 'string', self::OPTIONAL => true],
            'firstName' => [self::TYPE => 'string', self::OPTIONAL => true],
            'lastName' => [self::TYPE => 'string', self::OPTIONAL => true],
            'streetName' => [self::TYPE => 'string', self::OPTIONAL => true],
            'streetNumber' => [self::TYPE => 'string', self::OPTIONAL => true],
            'additionalStreetInfo' => [self::TYPE => 'string', self::OPTIONAL => true],
            'postalCode' => [self::TYPE => 'string', self::OPTIONAL => true],
            'city' => [self::TYPE => 'string', self::OPTIONAL => true],
            'region' => [self::TYPE => 'string', self::OPTIONAL => true],
            'state' => [self::TYPE => 'string', self::OPTIONAL => true],
            'country' => [self::TYPE => 'string'],
            'company' => [self::TYPE => 'string', self::OPTIONAL => true],
            'department' => [self::TYPE => 'string', self::OPTIONAL => true],
            'building' => [self::TYPE => 'string', self::OPTIONAL => true],
            'apartment' => [self::TYPE => 'string', self::OPTIONAL => true],
            'pOBox' => [self::TYPE => 'string', self::OPTIONAL => true],
            'phone' => [self::TYPE => 'string', self::OPTIONAL => true],
            'mobile' => [self::TYPE => 'string', self::OPTIONAL => true],
            'email' => [self::TYPE => 'string', self::OPTIONAL => true],
            'additionalAddressInfo' => [self::TYPE => 'string', self::OPTIONAL => true],
            'fax' => [static::TYPE => 'string', self::OPTIONAL => true],
            'externalId' => [static::TYPE => 'string', self::OPTIONAL => true],
            'custom' => [static::TYPE => CustomFieldObject::class, self::OPTIONAL => true]
        ];
    }
}

Stack information (please complete the following information):

Additional context Please implement the same for other classes too.

jenschude commented 2 years ago

Ui. Nice catch.

I just looked up and saw that the optional flag wasn't used anywhere. So I think it would make more sense to declare fields optional by default. And mark required fields not optional.

raju-cimpress commented 2 years ago

Yes, even that would help

jenschude commented 2 years ago

I just tagged a v2.17.1-beta.2 with the change.

In order to be correct we will have to mark all required fields which could take some time.

jenschude commented 2 years ago

Was now going to mark the fields as optional as it will not introduce a behavioral change and there were less files to be changed 😄 . The flag is now also automatically checked against the RAML specificiation to be there.

Please see https://github.com/commercetools/commercetools-php-sdk/pull/705

jenschude commented 2 years ago

I released version 2.18.0 which has the correct set optional flags