Skip to content

PHP 8.1 compatibility#372

Merged
treffynnon merged 2 commits intoj4mie:masterfrom
aaronpk:master
Jul 18, 2022
Merged

PHP 8.1 compatibility#372
treffynnon merged 2 commits intoj4mie:masterfrom
aaronpk:master

Conversation

@aaronpk
Copy link
Copy Markdown
Contributor

@aaronpk aaronpk commented Mar 26, 2022

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

@gbcox
Copy link
Copy Markdown

gbcox commented May 3, 2022

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
Copy link
Copy Markdown

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
Copy link
Copy Markdown

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

@treffynnon
Copy link
Copy Markdown
Collaborator

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
Copy link
Copy Markdown

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

@yan12125
Copy link
Copy Markdown

yan12125 commented Jul 12, 2022

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.

"phpunit/phpunit": "^4.8",

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

@aaronpk
Copy link
Copy Markdown
Contributor Author

aaronpk commented Jul 12, 2022

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
Copy link
Copy Markdown
Collaborator

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
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Collaborator

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 treffynnon merged commit efc8ea0 into j4mie:master Jul 18, 2022
@treffynnon
Copy link
Copy Markdown
Collaborator

Thanks @aaronpk - the changes are now merged

@aaronpk
Copy link
Copy Markdown
Contributor Author

aaronpk commented Jul 18, 2022

Thank you! 🚀

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants