Skip to content
This repository was archived by the owner on May 2, 2023. It is now read-only.

Add PHP 8.0 compatibility, fix iterating JSArray #59

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

mdio
Copy link
Contributor

@mdio mdio commented Jul 7, 2022

Due to the usage of each() in JSArray::valid() the internal array pointer would be advanced every time valid was called.
This resulted in every odd element to be skipped (e.g. 1st, 3rd, 5th...) when iterating.

Fixes #56

@vierbergenlars
Copy link
Owner

Can you please rebase on master?

Then the tests will be run by GH actions.

Due to the usage of each() in JSArray::valid() the internal array pointer would be advanced every time valid was called.
This resulted in every odd element to be skipped (e.g. 1st, 3rd, 5th...) when iterating.
@mdio mdio force-pushed the php-8-support branch from 9f5a5c3 to 0c34a4a Compare July 7, 2022 12:29
@mdio
Copy link
Contributor Author

mdio commented Jul 7, 2022

After installing dependencies this fails with denied: installation not allowed to Write organization package while trying to publish a Docker container to the GitHub registry to run the tests in.
The ones for 7.3 and 7.4 already exist so it works there.
I have no how to fix this or how this is even supposed to work.
Maybe it's not possible to add new test targets in a PR from a fork?
It surely works on our fork with that config.

@vierbergenlars
Copy link
Owner

Yes, actions that are triggered by a PR from ourside an organization (or author) run with a github token that only has public read access (obviously, or you would be able to write to my container namespace)

I'll try to look into how to run some builds without needing to push a docker image to a registry.

@mdio
Copy link
Contributor Author

mdio commented Jul 7, 2022

Thank you for looking into that. I tried to search for it but I couldn't find a solution for it.

A temp fix I could think of is that you add the PHP 8.0 and 8.1 entries to the tests.yml in your repo.
That would of course mean that you get failing tests for that commit because PHPUnit 4 does not work with PHP 8.0.

Also JFYI: I will be on vacation for 2 weeks from tomorrow on so I won't be able to work on my fork in the meantime or answer here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants