Skip to content

Conversation

@tacman
Copy link

@tacman tacman commented Jan 30, 2024

Description:

Please include a description of this change and which issue it fixes. If no issue exists yet please include context and what problem it solves.

Review

@sgiehl
Copy link
Member

sgiehl commented Feb 6, 2024

Some tests seem to be failing. Maybe newer version aren't compatible with our code 🤔

@tacman
Copy link
Author

tacman commented Feb 6, 2024

Yes, it appears to fail in PHP 7.2, which is so old it doesn't even show up on https://www.php.net/supported-versions.php

How about releasing a version 7 of this library that drops support for anything EOL? A bonus is that then the code could up updated to use some of the goodness that comes with PHP 8.1+, like typehints and property promotion. Anyone still using PHP 7.2 could continue to use version 6.

@Simbiat
Copy link

Simbiat commented Feb 6, 2024

I guess that would be another reason for new major version added to the list in #7544

@sgiehl
Copy link
Member

sgiehl commented Feb 6, 2024

As this library is a mandatory part of Matomo, we currently need to support the same PHP versions. Matomo is still compatible with 7.2, so for now we can't change that easily without additional effort.

Anyway, I guess the problem is located in our test action. It currently installs composer dependencies without respecting the platform requirements. Maybe removing this might solve it, as it should then only install a symfony version that is compatible with the used PHP version. See https://github.com/matomo-org/device-detector/blob/master/.github/workflows/phpunit.yml#L49

@tacman
Copy link
Author

tacman commented Feb 6, 2024

Yes, I believe that would work, though I'm still learning how to properly set up the matrix for github actions.

@sgiehl
Copy link
Member

sgiehl commented Feb 6, 2024

you can try changing the composer installs in the linked file above and remove the --ignore-platform-reqs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants