mirakl / sdk-php-shop

Mirakl PHP SDK for sellers
29 stars 16 forks source link

MiraklCollection Declaration is not compatible with php 7.4 #42

Closed Acrylangel closed 1 year ago

Acrylangel commented 1 year ago

Since the composer.json of the project is stating php >= 7.2.2 we are currently using the mirakl sdk in a php 7.4 project.

The problem here is that the File

Mirakl\Core\Domain\Collection\MiraklCollection implements the ArrayAccess Interface which defines some methods differently as in php 8.0 and onward, thus making the integration on php 7.4 impossible with an error message:

PHP Fatal error: Declaration of Mirakl\Core\Domain\Collection\MiraklCollection::offsetSet(Mirakl\Core\Domain\Collection\mixed $offset, $value): void must be compatible with ArrayAccess::offsetSet($offset, $value)

You should either return the implementation of this specifc Class back to its original State or bump the requirements up to php 8.0 and higher.

jreinke commented 1 year ago

Hello,

Which version of the SDK are you using please? Compatibility with PHP 8 has been added since v1.15.0 and should work with both PHP 7.2+ and PHP 8.

Acrylangel commented 1 year ago

well, first of all thank you for your answer. I was using the latest version (1.18) and as I said the problem arises in the class mentioned above as the changes made there regarding mixed typehintings are not supported in php 7.4.

For reference here is one of the methods that are not working in php 7.4:

     /**
     * @inheritdoc
     */
    public function offsetSet(mixed $offset, $value): void
    {
        $this->items[$offset] = $value;
    }

The method from the ArrayAccess Interface in php 7.4 is using the following body:

public function offsetSet($offset, $value)

jreinke commented 1 year ago

Actually, PHP 7.4 version should use the following file for MiraklCollection class: src/7.x/Mirakl/Core/Domain/Collection/MiraklCollection.php

How did you install the SDK please? With Composer?

The autoload is modified on the fly for custom PHP 7.x class thanks to this file: src/Mirakl/autoload.php

Acrylangel commented 1 year ago

Yes I am using composer and the autoload.php gets included in my main bootstrap.php the same way as always: include_once __DIR__.'/../vendor/autoload.php'; I am really not sure why we got this error message then:

PHP Fatal error: Declaration of 
Mirakl\Core\Domain\Collection\MiraklCollection::offsetSet(Mirakl\Core\Domain\Collection\mixed $offset, $value): void
must be compatible with ArrayAccess::offsetSet($offset, $value) 
jreinke commented 1 year ago

Did you execute the composer update with PHP 7.4?

Acrylangel commented 1 year ago

i am using the following setting in my composer.json to force the environment to php 7.4

  "config" : {
    "allow-plugins": {
      "ocramius/package-versions": true
    },
    "platform": {
      "php": "7.4.3"
    }
  }

After dumping the modified autoloader it appears as if there are 2 psr-4 folders associated with the Mirakl namespace.

["Mirakl\"]=> array(2) { 
    [0]=> string(107) "{redacted}/vendor/mirakl/sdk-php-shop/src/Mirakl/../7.x/Mirakl/" 
    [1]=> string(104) "{redacted}/vendor/composer/../mirakl/sdk-php-shop/src/Mirakl" 
}

maybe thats the issue. I am not so well versed in the internal composer autoloading mechanisms to know if this could cause an issue on a php 7.4 platform

Acrylangel commented 1 year ago

Okay i have tested this again in a dockerized php 7.4 environment. I have instantiated the MiraklCollection Class with no problems. I have also used the aformentioned method offsetSet without any problems.

Maybe you are right and I somehow butchered my vendor folder because before trying that i completely removed it and ran composer update with the latest version of this package.

I'll close this issue as it is actually working as intended and it seems I did something wrong.

jreinke commented 1 year ago

Thanks for your feedback!