Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP 8 support #2388

Closed
4 tasks done
ssddanbrown opened this issue Nov 28, 2020 · 7 comments
Closed
4 tasks done

PHP 8 support #2388

ssddanbrown opened this issue Nov 28, 2020 · 7 comments

Comments

@ssddanbrown
Copy link
Member

ssddanbrown commented Nov 28, 2020

Now that PHP 8 has been released we should update the app to work on php8.

Laravel framework v6.20 adds PHP8 support: https://github.com/laravel/framework/releases/tag/v6.20.0
This does bump the minimum version of php to 7.2.5: laravel/framework#34928

  • Update framework.
  • Go through dependencies and update where require.
  • Update GitHub actions to cover php 8.
  • Test locally

Due to php version change we'll need to add a notice to the update docs on release.

@ssddanbrown
Copy link
Member Author

Removed from being targeted on next release due to outstanding issues/work in #2410. Don't want to rush it.

@JtheBAB
Copy link

JtheBAB commented Dec 15, 2020

FYI:
The current Debian Version Buster has PHP 7.3

Ubuntu 18.04 (Support until 2028) has PHP 7.2

@sid606
Copy link

sid606 commented Jan 19, 2021

So we can't use it on php8?

@AlphaJack
Copy link

@sid606 Not yet

@p1xelshader
Copy link

@ssddanbrown Not sure if this is the right place for this Dan, but it's related to the lack of PHP 8 support and it may help someone so here goes. I updated packages on Ubuntu 18.04 today and it automatically installed PHP 8. I subsequently tried to update BookStack and hit a wall.

I fixed this by setting Apache PHP back to 7.4:

sudo a2enmod proxy_fcgi setenvif
sudo a2enconf php7.4-fpm
sudo systemctl reload apache2

And also setting PHP CLI back to 7.4 and masking 8.0:

sudo rm /etc/alternatives/php; sudo ln -s /usr/bin/php7.4 /etc/alternatives/php;
sudo systemctl mask php8.0-fpm

Other people may hit this issue before PHP 8 support is available, so maybe worth including a note in the admin docs?

Thanks again for your work, we all love BookStack here!

@ssddanbrown
Copy link
Member Author

Thanks @p1xelshader. Ubuntu should not have automatically updated to PHP8 itself, I'm thinking at some point you may have added another repository and installed either php8 itself, or software that requires php8, and that has become active on update? Otherwise 18.04 should remain on php7.2 unless extra software is added.

Yeah, Could be worth adding a note in the requirements on this.


Re-review

Just had another dive into this and it remains tricky. I think dependencies are about there now but the way that composer ideally works is not set-up for the scenario that BookStack faces. Our options appear to be:

  • Remove composer.lock from the repo.
    • Could keep supporting PHP 7.2.
    • Deal with the variance that may occur in installed dependencies due to this.
    • Technically less secure I guess?
    • Slower composer install times.
    • Would have to deal with people attempting to add it back in (Seen on other projects).
  • Advise php8+ people to composer update.
    • Messy since it'll alter composer.lock, which will likely cause issues on later updates via git.
    • Could keep supporting PHP 7.2.
  • Update min PHP version.
    • It's only been a year since our last major change (php7.0 to php7.2). Can have impact on user base.
    • Would need to update scripts and provide upgrade assistance.
    • No composer.lock issues.
    • Likely pre-prepared for next Laravel LTS version if we jump to 7.4 (Not a guarantee).
    • Would be nice to be on a more recent version.

Shame there's no way to define multiple composer.lock files for different PHP versions.
I'm tempted to go with the PHP version change. Sooner than expected but will need to be done at some time, Would probably jump to 7.4 (min) to gain some nice language features while potentially hopefully getting a longer stable cycle until the next change.

@ssddanbrown
Copy link
Member Author

Done, as part of the commits listed within #2648 (comment)

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

Successfully merging a pull request may close this issue.

5 participants