search

AmpersandHQ / magento2-disable-stock-reservation

This module disables the inventory reservation logic introduced as part of MSI in Magento 2.3.3
GNU Lesser General Public License v3.0
211 stars 60 forks source link

PHP 8.2 Support #116

Closed sprankhub closed 1 year ago

sprankhub commented 1 year ago

Since Magento 2.4.6 supports PHP 8.2, it would be great to also support it in this extension :)

convenient commented 1 year ago

It's just missing from the composer.json require section correct?

Screenshot 2023-05-25 at 09 16 28

The suite of tests runs 2.4.6 and PHP 8.2 fine, but maybe I'm not capturing something correctly @sprankhub ?

sprankhub commented 1 year ago

The tests currently do not run on PHP 8.2 if I am not mistaken. You currently run them on PHP 8.1 for all Magento versions. That is why I did not simply send a PR for composer.json :)

convenient commented 1 year ago

@sprankhub The tests are ✨ groovy ✨ to be able to fully spin up magento for the API driven tests locally as well as in CI

Each entry listed in travis uses PHP 8.1 to run composer install and do some of the static analysis tests, it then uses docker to spin up more environment specific mysql/elasticsearch/php/composer for each version of magento.

The full installation of magento used by travis (or locally) is https://github.com/AmpersandHQ/magento2-disable-stock-reservation/blob/bdb5cb2d339204cd5582600f076162e2b40b48d3/.travis.yml#L32

Which is defined as https://github.com/AmpersandHQ/magento2-disable-stock-reservation/blob/bdb5cb2d339204cd5582600f076162e2b40b48d3/composer.json#L41-L44

This ultimately boots a magento docker instance defined as https://github.com/AmpersandHQ/magento-docker-test-instance/blob/fd18543e4730ce8932adfe8d297fa070405af290/Makefile#L55-L56

See in this video the 2.4.0 tests use PHP 7.4, and the 2.4.6 tests use PHP 8.2

https://github.com/AmpersandHQ/magento2-disable-stock-reservation/assets/600190/13b172f4-356b-4442-a59c-f0c6ebd3a430

So yeah, are you seeing a bug / problem with the current setup? Tbh i'm not sure why we're not getting errors when we composer require it in as it doesn't say 8.1 or 8.2 in the composer.json file 😅

sprankhub commented 1 year ago

Oh wow, okay, thanks for the explanation. I did not dig too deep. I just saw this:

https://github.com/AmpersandHQ/magento2-disable-stock-reservation/blob/bdb5cb2d339204cd5582600f076162e2b40b48d3/.travis.yml#L2

And thought you only run it on 8.1.

And oh boy, I was just too dumb to properly understand the dependencies as well. ^8.0 does fulfill 8.2, so all good! Sorry for the false alarm!

convenient commented 1 year ago

@sprankhub No probs!

Tbh putting this inside docker makes it a bit easier for us to manage the deps, stops random CI issues causing some php libraries to become unavailable etc and allows us to spin it up locally as well.