Conversation
|
Hello, I'm testing this patch and noticed the following error:
This appears to be corrected by simply removing "Serializable" from line: 2409 class IdiormResultSet implements Countable, IteratorAggregate, ArrayAccess { |
|
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)
IdiormResultSet does implement __serialize and __unserialize, so there shouldn't be deprecation messages. |
|
is there any progress expected on merging this? i'd rather not fork idiorm but all signs point to functionally dead upstream. |
|
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. |
|
you've decided to not merge a valid PR because your infrastructure is broken and you're unable to fix it? simply incredible. |
|
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. Line 35 in d23f970 |
|
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. |
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
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. |
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. |
a33a407 to
849bcc3
Compare
|
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 PHPUnit 4.8 won't run on PHP8 because it uses the |
|
Thanks @aaronpk - the changes are now merged |
|
Thank you! 🚀 |
This PR recreates the functional changes in #370 to support PHP 8.1 without the blank space changes.