search

gnikyt / laravel-shopify

A full-featured Laravel package for aiding in Shopify App development
MIT License
1.24k stars 374 forks source link

Force composer to choose <=10.0.5 for the Basic Shopify API #1201

Closed Kyon147 closed 2 years ago

Kyon147 commented 2 years ago

Hey @osiset - need to set the API version as your recent merge breaks the tests for the APIHelper as they all reference ResponseAccess but one of the methods has become mixed so it throws an error.

Let me know if you are happy for this as a quick fix.

Once this is merged I can release the new version 👍

gnikyt commented 2 years ago

@Kyon147 fine with this merge, but what is it long term we can do? People on 8.1 we're getting a breaking error without the mixed.

gnikyt commented 2 years ago

offsetGet and current, are methods in ResponseAccess which have added mixed. Tests passed for 7.x and 8.x, so I am guessing we just need to adjust the tests' stub?

Kyon147 commented 2 years ago

Yeah that makes sense, this is the full error that is happening. I'm trying to get the error to happen locally but can't seem to replicate it outside of the automated test at the moment. 

1) Osiset\ShopifyApp\Test\Actions\ActivatePlanTest::testRunRecurring
TypeError: Return value of Osiset\BasicShopifyAPI\ResponseAccess::offsetGet() must be an instance of Osiset\BasicShopifyAPI\mixed, instance of Osiset\BasicShopifyAPI\ResponseAccess returned

/home/runner/work/laravel-shopify/laravel-shopify/vendor/osiset/basic-shopify-api/src/ResponseAccess.php:67
/home/runner/work/laravel-shopify/laravel-shopify/src/Services/ApiHelper.php:248
/home/runner/work/laravel-shopify/laravel-shopify/src/Actions/ActivatePlan.php:115
/home/runner/work/laravel-shopify/laravel-shopify/tests/Actions/ActivatePlanTest.php:54
phpvfscomposer:///home/runner/work/laravel-shopify/laravel-shopify/vendor/phpunit/phpunit/phpunit:97
gnikyt commented 2 years ago

@Kyon147 Its because of this.

mixed was not introduced until PHP 8.0

offsetGet, etc are now using mixed.. so its expecting ResponseAccess to follow the interface.

But by following the interface, we kill usage of < 8.0.

I think were at a cross roads here... if people want to use PHP 8.0, we may have to update this package to focus on PHP 8 and disregard older versions, which sucks.. but we are now stuck in compatibility issues.

Kyon147 commented 2 years ago

Ah yeah it's 7.x that's the issue - was on php8 on my local.

I think it makes sense to move over to PHP 8, you can only hold onto backward compatibility for so long.

If someone really wants to use PHP7 still considering Laravel 9 now requires PHP8 as a minimum they would need to use an older version of this package anyways or make a fork to suit their needs.

gnikyt commented 2 years ago

@Kyon147 Sadly, I just tried to google all I can to see if we can somehow support the return type, but it sees it as something custom, as if mixed was an interface or class or something.

My proposal, if you think its fine:

  1. Merge in your PR to force API library to <= 10.0.5
  2. Release a patch of this package for it
  3. Make a new PR for PHP v8.0 support, in-tandem deleting any PHP 7.X support
  4. Review PR for step 3... if good... then we merge and release as a new major to keep things seperate
  5. Update the wiki to state if PHP 7, you need this, if PHP 8, use this version, etc.
Kyon147 commented 2 years ago

@osiset - makes total sense to me - it also means that all the recent fixes around billing and other bits are included for those on PHP7.x which is nice.

I'll merge this PR in for version v17.2.0 shortly and then v18.0 can be the one to support PHP8.0 explicitly and can update the workflows etc for v18 to only run the PHP8 flows and composer support for ^10.0.6 of the api to.