PHP 8 support

[elrido]

TL;DR: It seems that our code base still works unchanged in PHP 8, if our phpunit tests are to be trusted. They provide 97.6% code coverage (1266 of 1297 lines).

PHP 8 is announced to be released end of November 2020, so I was curious if it's breaking changes have any impact on our code base or if we can still support older PHP releases.

As you may have seen I created a php8 branch for my experiments. The result is that the existing unit tests pass with no errors or warnings on a php 8.0 beta release and do so without any changes to our PHP code base (the lib and vendor folders). There are a few "BUT"s further explained below.

In order to test this, I had to enable PHP 8.0 (beta) in the github action workflow. In that environment phpunit is installed using composer - each phpunit release on packagist except the latest version 9 is capped at 7.x (^7 in terms of their composer.json definition). So to test PHP 8 support we can only use phpunit 9 at this time (or we need to install it in a different way, but that would get messy). Unfortunately version 9 also introduced several breaking changes that make it incompatible with even it's predecessor version 8. The branch contains changes in the tests and the phpunit.xml to make them work with phpunit version 9 - the logic of the tests remains unchanged.

The downside of these changes is that phpunit 9 is the only version working with those updated tests. I wouldn't mind that so much, but version phpunit 9 requires PHP 7.3 or later to work. This means on that branch we loose the test coverage for PHP 5.6, 7.0, 7.1 and 7.2. We know that the code still works on those branches, because we haven't changed it and in master it still passes the tests in all of these versions.

The benefit of using a phpunit version that supports PHP 5.6 is that we would notice if a change in our PHP code breaks compatibility to that version. Unfortunately phpunit 5.7 is the last version with PHP 5.6 support, which is why we are still stuck on that release with the unit tests. It was still fully working with PHP 7.4 and so does let us test the full range of PHP versions, that we support, with the same tests.

Due to the above issues, we can't have a single version of our phpunit tests that will work in all PHP versions supported by us, anymore. I propose to keep the php8 branch around, so we can use it to test changes in our PHP code against PHP 8, if they occur, but otherwise keep the existing tests in the master branch unchanged, so we do detect backwards incompatible changes going forward. Once we do drop support for PHP < 7.3 in our code, for whatever reason, we can merge the changes of the php8 branch into master and switch to the newer phpunit version.

[rugk]

Well… PHP v7.2 will be at the end of life in Jan 2021, anyway. So hmm… dunno.

[elrido]

Well, thats all nice, but the reality is this:

• RHEL 7 will still be maintained until August 2021 and ships PHP 5.4 (which we don't support anymore as of 1.3)
• Ubuntu 14.04 LTS server will still be maintained until 2022 and ships PHP 5.5 (1.3 will work, but not the latest 1.3.4)
• SLES 11 will still be maintained until 2022 and ships PHP 5.6
• Debian 9 will still be maintained until 2022 and ships PHP 7.0

Of course all of these distros have later releases with more up to date packages and often (community) repos with backports of later PHP releases. It's just a reality that in the wild you will encounter these old releases and so I would rather have these folks be able to update to a later PrivateBin release then sticking to an old release of ours, because their server runs on a legacy distro - which often won't be under the control of the PrivateBin admin, but whatever their hoster provides them with. Hence I am for server applications very much in favour of curating docker images, where we can use a well supported stack of software (our alpine 3.12 image ships with PHP 7.3).

[elrido]

Update on this: With alpine 3.13 we get PHP 8.0 packages (they also ship PHP 7.4, so you can choose and even install both, if needed) and so far I didn't encounter any issues with it. I am working on updating the image to use it and will switch the demo instance to it, once it's published. So it really looks like our tests in the php8 branch didn't lie and it just works.

[rugk]

So in the docker we already use PHP 8.0, so what's still missing here?

I noticed, we should really include it in our test CI, taht is one big point:

PrivateBin/.github/workflows/tests.yml

Line 18 in fba8384

php-versions: ['5.6', '7.0', '7.1', '7.2', '7.3', '7.4']

Is bumping the versions there enough?
#839

On GitHub I noticed there are stil some diffs/commit between the old php8 branch and the master, so what is the thing about these? Do we need these, @elrido?

https://github.com/PrivateBin/PrivateBin/compare/php8

[rugk]

Ah okay, it was backwards-compatibility, I see. Though Debian 9 is only supported as an LTS/"Stretch" version until June 2022, so AFAIK we can soonish upgrade.

Especially PrivateBin/docker-nginx-fpm-alpine#73 may be a nice thing to have.

[elrido]

It also affects earlier versions of php 7, so it would be way beyond 2022, realistically. What if we move the modified tests into a different folder (also flagged in .gitattributes from being exported) and change the github action to run the "old" phpunit version in the current tst folder for php < 7.3 and the newer release of phpunit in the new folder.

It would mean that changes in the unit tests have to get replicated to both code bases, but then we could cover the newer versions as well.

[rugk]

Hmm, does not really sound like a good thing IMHO and goes against the git-intended development workflow.
Then rather periodically merge master into php8, so that is also tested and up-to-date?

Just like to look at how other projects do it so Nextcloud seems to support PHP 7.3-8.0, so hmm yeah…
But is it really only the tests? No other changes are required?

Then this is indeed really unfortunate?
Then we could just maybe setup a CI at the php8 branch too and there test, well… only php 8 (or is it backwards-compatible?).

[elrido]

But is it really only the tests? No other changes are required?

This is really just phpunit and their decision to only support certain versions of PHP. I was very pleased to see that due to our use of best practices in PHP coding it worked out of the box and with no warnings. Our code plays nice on all versions of PHP from 5.6 - 8.x, while the newer phpunit releases do not support even early PHP 7 releases.

Then we could just maybe setup a CI at the php8 branch too and there test, well… only php 8 (or is it backwards-compatible?).

It already is, but of course it will only run when changes get pushed into that branch. As a maintainer, you can manually re-run the checks on the last commit.

Have a look at the branch comparison with master:
https://github.com/PrivateBin/PrivateBin/compare/php8

You'll see that nearly all changes were in tst, due to the API changes that the new phpunit required (these tests are not backwards compatible, so we can't run the updated test code on the older phpunit and vice versa) and the composer dependencies and the github action got changed. No change in the PHP code residing in lib required and as I explained, since last year we have been publishing our container images with PHP 8 by default and on neither the demo site nor my personal instance did I find any PHP warnings or errors reported that relate to PHP 8 in all that time.

We might be able to separate the phpunit framework boilerplate from the actual test code and then import the test code from a single location into both framework classes, but that would be even clunkier then keeping a separate folder with duplicated logic and require a lot of work to set up as well.

---

I don't see a reason why we should prevent someone from running the latest version of our application on an old PHP release, as long as there is no functional reason to drop support for that version. The risks involved with doing so are not our responsibility. I've seen plenty of businesses having very lax upgrade policies as long as the servers are only accessible internally. Same goes for hobbyists that run some Raspberry Pi 1 or 2 that they installed in 2015 in their home network with PHP 5.6. I run some SOCs with older Raspbian releases myself for SIMH emulating PDP-8/10/11 - these are only accessible internally or via ssh gateway running the latest stable alpine release and I can't upgrade them without rebuilding SIMH, which was a PITA to get compiled and tweaked for these 1970s OS's running on them in the first place. Keeping legacy software running, off the net, for fun or profit, is a legitimate use case.

At the same time, I'm very keen on ensuring our project runs on the latest PHP release and browser versions for production use in the harsh internet environment, which is why I did put the effort into validating PHP 8 as early as possible. I'd have done it even earlier with a beta release, but had to wait on github actions to support it using a release candidate. Dropping support for old releases would make sense to me, if a new PHP release introduces breaking changes or a library we use, that can't be easily replaced, drops support for older releases.

[rugk]

It already is, but of course it will only run when changes get pushed into that branch. As a maintainer, you can manually re-run the checks on the last commit.

Wait a second, I have an idea… let's setup a CI that merges master into php8. I tried doing that: #843

😄

Let's hope we get no merge conflicts, and if we get… we get to that at least.

I don't see a reason why we should prevent someone from running the latest version of our application on an old PHP release, as long as there is no functional reason to drop support for that version.

Yes, if we don't have to sacrifice coding style or (PHP) features I fully agree. That's why I asked. As it only concerns tests here, I fully agree, let's keep it compatible, but lets also run our tests with PHP8.

At the same time, I'm very keen on ensuring our project runs on the latest PHP release and browser versions for production use in the harsh internet environment, which is why I did put the effort into validating PHP 8 as early as possible

I fully agree and thanks for that! It's really a good thing. 😃

[rugk]

So finally it works and our PHP8 branch is now automatically kept up-to-date with the current master.

Here is the PHP8 branch: https://github.com/PrivateBin/PrivateBin/tree/php8

The commits also show in ticks what CI ran and I'm quite unsure. The latest merge commit is only covered by StyleCI: https://github.com/PrivateBin/PrivateBin/commits/php8

Maybe GitHub does have a "prevent infinitive loop of CI pushes and CI starts" feature and thus prevents this start? I checked the settings but could not find anything like that hmm… 🤔

[elrido]

Our test pipeline is triggered by push events. It seems the merge commit performed by github wasn't "pushed". That would either imply the action of the bot is performed on the live repo or the "push" is only triggered if it is tied to a human user account? Or it is as as you suspect and they don't consider it in the push events to avoid loops.

I couldn't find anything in the docs either, but we could of course append a matrix task after the merge&push of the php8 branch (for php 8.0, 8.1, ...):

PrivateBin/.github/workflows/tests.yml

Lines 16 to 95 in 3a801bc

	PHPunit:
	runs-on: ubuntu-latest
	strategy:
	matrix:
	php-versions: ['7.3', '7.4', '8.0']
	name: PHP ${{ matrix.php-versions }} unit tests on ${{ matrix.operating-system }}
	env:
	extensions: gd, sqlite3
	extensions-cache-key-name: phpextensions
	steps:
	# let's get started!
	- name: Checkout
	uses: actions/checkout@v2
	# cache PHP extensions
	- name: Setup cache environment
	id: extcache
	uses: shivammathur/cache-extensions@v1
	with:
	php-version: ${{ matrix.php-versions }}
	extensions: ${{ env.extensions }}
	key: ${{ runner.os }}-${{ env.extensions-cache-key }}
	- name: Cache extensions
	uses: actions/cache@v2
	with:
	path: ${{ steps.extcache.outputs.dir }}
	key: ${{ steps.extcache.outputs.key }}
	restore-keys: ${{ runner.os }}-${{ env.extensions-cache-key }}
	- name: Setup PHP
	uses: shivammathur/setup-php@v2
	with:
	php-version: ${{ matrix.php-versions }}
	extensions: ${{ env.extensions }}
	# Setup GitHub CI PHP problem matchers
	# https://github.com/shivammathur/setup-php#problem-matchers
	- name: Setup problem matchers for PHP
	run: echo "::add-matcher::${{ runner.tool_cache }}/php.json"
	- name: Setup problem matchers for PHPUnit
	run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json"
	# composer cache
	- name: Remove composer lock
	run: rm composer.lock
	- name: Get composer cache directory
	id: composer-cache
	run: echo "::set-output name=dir::$(composer config cache-files-dir)"
	# http://man7.org/linux/man-pages/man1/date.1.html
	# https://github.com/actions/cache#creating-a-cache-key
	- name: Get Date
	id: get-date
	run: |
	echo "::set-output name=date::$(/bin/date -u "+%Y%m%d")"
	shell: bash
	- name: Cache dependencies
	uses: actions/cache@v2
	with:
	path: ${{ steps.composer-cache.outputs.dir }}
	key: ${{ runner.os }}-composer-${{ steps.get-date.outputs.date }}-${{ hashFiles('**/composer.json') }}
	restore-keys: ${{ runner.os }}-composer-${{ steps.get-date.outputs.date }}-
	# composer installation
	- name: Setup PHPunit
	run: composer install -n
	- name: Install Google Cloud Storage
	run: composer require google/cloud-storage
	# testing
	- name: Run unit tests
	run: ../vendor/bin/phpunit --no-coverage
	working-directory: tst

[rugk]

I couldn't find anything in the docs either, but we could of course append a matrix task after the merge&push of the php8 branch (for php 8.0, 8.1, ...

Hmm, how?

I rather hoped the push in the php8 branch would trigger the yaml pipeline of that branch, which already contains php8 in the matrix there.