j4mie / idiorm

A lightweight nearly-zero-configuration object-relational mapper and fluent query builder for PHP5.
http://j4mie.github.com/idiormandparis/
2.01k stars 369 forks source link

PHP 8.1 compatibility #372

Closed aaronpk closed 2 years ago

aaronpk commented 2 years ago

This PR recreates the functional changes in #370 to support PHP 8.1 without the blank space changes.

gbcox commented 2 years ago

Hello, I'm testing this patch and noticed the following error:

E_DEPRECATED (8192) vendor/j4mie/idiorm/idiorm.php:2405 IdiormResultSet implements the Serializable interface, which is deprecated. Implement serialize() and unserialize() instead (or in addition, if support for old PHP versions is necessary)

This appears to be corrected by simply removing "Serializable" from line: 2409 so the line then appears as:

class IdiormResultSet implements Countable, IteratorAggregate, ArrayAccess {

yan12125 commented 2 years ago

Hmm, I didn't get any deprecation message on PHP 8.1.7. Per https://php.watch/versions/8.1/serializable-deprecated (emphasis are mine)

implementing the Serializable interface without implementing serialize and unserialize methods is deprecated.

IdiormResultSet does implement serialize and unserialize, so there shouldn't be deprecation messages.

cthu1hoo commented 2 years ago

is there any progress expected on merging this? i'd rather not fork idiorm but all signs point to functionally dead upstream.

treffynnon commented 2 years ago

There is an outstanding issue with this project - Travis CI is no longer connected to the repository. For this to be merged it needs to pass a CI build. To enable this we probably need to convert the project to use GitHub Actions, but getting this working with PHP 5.2 is non-trivial and not something I have had time to get working at present.

cthu1hoo commented 2 years ago

you've decided to not merge a valid PR because your infrastructure is broken and you're unable to fix it? simply incredible.

yan12125 commented 2 years ago

Apparently PHP 5.2 support was not properly tested since the Travis CI age. idiorm uses phpunit ^4.8, and the latter requires php >= 5.3.3.

https://github.com/j4mie/idiorm/blob/d23f97053ef5d0b988a02c6a71eb5c6118b2f5b4/composer.json#L35

https://github.com/sebastianbergmann/phpunit/blob/4.8.0/composer.json#L24

aaronpk commented 2 years ago

If I show a successful test result would you consider merging this? I can probably get the tests running locally enough to show they pass.

treffynnon commented 2 years ago

you've decided to not merge a valid PR because your infrastructure is broken and you're unable to fix it? simply incredible. -- @cthu1hoo

  1. it is the project's infrastructure not mine
  2. it is not automatically my responsibility to fix or maintain it
  3. where is your helpful PR to along with your distinctly unhelpful comments?

Apparently PHP 5.2 support was not properly tested since the Travis CI age. idiorm uses phpunit ^4.8, and the latter requires php >= 5.3.3. -- @yan12125

This is not actually correct. PHPUnit 3.6 is employed for testing on PHP 5.2: https://github.com/j4mie/idiorm/tree/master/test/docker_for_php52

If I show a successful test result would you consider merging this? I can probably get the tests running locally enough to show they pass. -- @aaronpk

You should be able to use the docker image (I have not built it in a while so assuming all the sources are still valid): https://github.com/j4mie/idiorm/tree/master/test/docker_for_php52 to show that and we can look at finally merging this PR.

yan12125 commented 2 years ago

This is not actually correct. PHPUnit 3.6 is employed for testing on PHP 5.2: https://github.com/j4mie/idiorm/tree/master/test/docker_for_php52

Got it. Thanks for the pointer. It will be better if the version listed in composer.json matches the one used in the Docker image.

On the other hand, I tried the Docker image. Fortunately the only breakage is that Ubuntu 12.04 is no longer supported. That can be fixed with:

diff --git a/test/docker_for_php52/Dockerfile b/test/docker_for_php52/Dockerfile
index f9640b6..b6ad5fb 100644
--- a/test/docker_for_php52/Dockerfile
+++ b/test/docker_for_php52/Dockerfile
@@ -4,6 +4,7 @@ FROM ubuntu:12.04

 RUN mkdir /php && \
     cd /php && \
+    sed -i "s#archive.ubuntu.com#old-releases.ubuntu.com#g" /etc/apt/sources.list && \
     apt-get update && \
     apt-get install -y --no-install-recommends \
         autoconf binutils build-essential bzip2 ca-certificates \

And tests are green with the built Docker image and this pull request.

PHPUnit 1.0.2 by Sebastian Bergmann.

Configuration read from /tmp/idiorm/phpunit.xml

...............................................................  63 / 140 ( 45%)
............................................................... 126 / 140 ( 90%)
..............

Time: 0 seconds, Memory: 9.00Mb

OK (140 tests, 171 assertions)
treffynnon commented 2 years ago

I have opened a PR that should make testing possible again. Unfortunately, the Idiorm test suite does not appear to be passing on PHPUnit 8.5 and therefore doesn't pass on PHP 8.0 and above. So the unit tests would need to be updated for this branch to be merged. The would need to pass on PHP 5.2, 5.4, 5.6, 7.0, 7.1, 7.2, 7.3, 7.4, 8.0, 8.2

5.2 runs on PHPUnit 3.6 5.4 -> 7.4 runs on PHPUnit 4.8 8.0 and above runs on PHPUnit 8.5

PHPUnit 4.8 won't run on PHP8 because it uses the each function which no long exists. Idiorms tests won't run on PHPUnit 8.5 because

PHP Fatal error:  Uncaught Error: Class "PHPUnit_Framework_TestCase" not found in /home/runner/work/idiorm/idiorm/test/CacheIntegrationTest.php:3
treffynnon commented 2 years ago

Thanks @aaronpk - the changes are now merged

aaronpk commented 2 years ago

Thank you! 🚀