Skip to content

Conversation

briandotdev
Copy link
Contributor

This PR adds PHP 8.0 support. I don't normally submit competing PRs however I had this 95% complete when the other PR came through and this is passing all tests whereas the other PR is silently failing the build of some modules that are not compatible with PHP 8. Significant changes are as follows:

Build/Test Level Changes

-Added 8.0 Dockerfiles (cli, chromium, fpm, alpine, alpine-lts)
-Added 8.0 Goss test identical to 7.4 test but removing Cassandra extension that will not be supported in PHP 8

PHP Internal Changes

-XMLRPC is removed from PHP core per here
-PHP 8 requires xdebug 3.0 per here
-PHP 8 requires phpredis 5.3 or higher per here
-Imagick extension supporting PHP 8 not yet released to PECL and installed from source per here
-AMQP extension supporting PHP 8 not yet released to PECL and installed from source per here
-XMLRPC extension supporting PHP 8 PECL package is broken and installed from source per here
-Cassandra extension removed from PHP 8 builds due to no plans to support PHP 8 per here
-Xdebug 3 changes config settings for enable code coverage per here

Note: At a future date a PR can be submitted to clean up/consolidate the build scripts once all these modules eventually make their way to PECL however building from source allows us to release a full feature PHP 8 image today.

Testing the PR

All of the new PHP 8 images were built and are passing all tests. I have also built and tested all PHP 7.2-7.4 images and ensured they are still passing all tests after modification of build scripts.

Additional Notes

The other PR bumps Node, NPM, and Composer to latest version however I felt this was outside the scope of this PR and have not included. I believe there is another PR already open for Composer or I am happy to submit additional PRs for these if needed in order to keep the scope of this change to PHP 8 only.

@Pezhvak
Copy link

Pezhvak commented Jan 15, 2021

so you just took my pr to create your own? wow dude fine

@Pezhvak Pezhvak mentioned this pull request Jan 15, 2021
@briandotdev
Copy link
Contributor Author

so you just took my pr to create your own? wow dude fine

Like I said, competing PRs is not something I normally do. As you can see, I put a lot of research into this and have been working it for a while. I did not download your source and copy your PR.

Your PR was silently failing the compile of multiple modules. I brought those up and your response was that I should do the testing and the container was working fine on your CI already. I was worried that if left uncorrected and your PR was adopted this would take the project backwards. So I gave you a few days to look into the issues I mentioned and then I offered up an alternative.

Nothing personal my friend. If you would like to incorporate the changes on the missing extensions I've got no issues closing this PR and supporting yours. 🙂

@Pezhvak
Copy link

Pezhvak commented Jan 15, 2021

@briandotdev you cannot test something that you haven't downloaded, after all it's open sourced, go ahead, it's all your. all i wanted in the end is to contribute to something that i once used. i appreciate your research and the time you put to gather this up. i just wished you did the same for me. best wishes

@briandotdev
Copy link
Contributor Author

i appreciate your research and the time you put to gather this up. i just wished you did the same for me. best wishes

I did. That was my first attempt.

#112 (comment)
#112 (comment)
#112 (comment)

@Pezhvak
Copy link

Pezhvak commented Jan 15, 2021

@briandotdev lets get this over with and get you merged, i cannot teach you ethics, its just something that you either have or you don't have. just for the sake of respecting your reply, i have to add that i told you that i can fix it if there is a problem #112 (comment)
if you liked to fix my pr you could just ask for it, i would be more than happy to give you access to my forked repo so you could change what is needed, that's how contributing works, we build on top of our colleges work, copying and making something entirely yours is just something we don't do. i really can't keep myself engaged with this, have lots to do. unsubscribing from this.

@polyskalov
Copy link

So, @edbizarro, can you merge this PR? Really looking forward to this update

Copy link

@wedi wedi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me. I'd just suggest you update the Readme to contain PHP 8, too.

@briandotdev
Copy link
Contributor Author

@wedi I've updated the Readme to reflect PHP 8.0. I've followed URL convention for badges but you guys will need to update those. I'm also seeing that some of the previous badges probably are needing an update also.

While updating the Readme had the realization that you have been depricating EOL PHP versions. PHP 7.2 could be removed as part of this PR or could be left to another PR in order to get this out the door. If that's the direction you want to go I'm happy to submit another PR or let someone else take a swing.

@edbizarro
Copy link
Owner

Thanks for the hard work guys!

@edbizarro edbizarro merged commit c67b5fb into edbizarro:master Jan 28, 2021
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