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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dropping PHP < 7.1 #3758

Merged
merged 11 commits into from Nov 29, 2018

Conversation

Projects
6 participants
@j0k3r
Copy link
Member

j0k3r commented Oct 24, 2018

Q A
Bug fix? no
New feature? kind of
BC breaks? kind of
Deprecations? no
Tests pass? yes
Documentation will need to be updated
Translation no
CHANGELOG.md no
Fixed tickets #...
License MIT
  • Sunsetting PHP 5.6 & 7.0 馃憢
  • Also cleanup some files.
  • And using @Daniel-KM PHPePub which remove some deprecated stuff and seems to be more up to date than the official version.
  • Upgrade wallabag/tcpdf to fix all issues we have (+ fixing CVE-2018-17057)
  • Use our own fork of CraueConfigBundle to fix utf8mb4

NelmioApiDoc bundle wasn't updated because the upgrade guide doesn't explain how to migrate but provide a command that totally screw up all our annotation.

SchebTwoFactorBundle 3 might need to be updated later (to avoid too much stuff in that PR)

@j0k3r j0k3r added this to the 2.4.0 milestone Oct 24, 2018

@j0k3r j0k3r added this to To Do in 2.4 via automation Oct 24, 2018

@j0k3r j0k3r requested a review from Kdecherf Oct 24, 2018

@Kdecherf

This comment has been minimized.

Copy link
Contributor

Kdecherf commented Oct 24, 2018

MySQL seems broken here

@j0k3r

This comment has been minimized.

Copy link
Member Author

j0k3r commented Oct 25, 2018

While it works pretty fine on my local machine 馃槙

@j0k3r

This comment has been minimized.

Copy link
Member Author

j0k3r commented Oct 29, 2018

@childnode

This comment has been minimized.

Copy link

childnode commented Nov 6, 2018

鈿狅笍
just to throw in.. as long conservative distributions like debian use a lower version in latest stable maintained release or LTS ... perhaps we should not force-update that forces users to backport packages to let some piece of software up to date?!
https://packages.debian.org/search?keywords=mod-php&searchon=names&suite=stable&section=all
https://packages.debian.org/search?keywords=php7&searchon=names&suite=stable&section=all

Ubuntu LTS trusty is stuck on 5.9
https://packages.ubuntu.com/search?suite=trusty&searchon=names&keywords=mod-php

xenial is on 7.0 while only latest bionic lts is on 7.2
referring to https://wiki.ubuntu.com/Releases they are all still valid supported, at least up to 2021 if you skip trusty!

@j0k3r

This comment has been minimized.

Copy link
Member Author

j0k3r commented Nov 7, 2018

I know that some people are still running obsolete stuff.

But some PHP versions are going to be dropped in the coming months: PHP 5.6 & 7.0: http://php.net/supported-versions.php
There'll be no more official active support and no more official security support.

Following that Composer PHP stats show that PHP 7.2 is rising and all other versions are declining.
https://blog.packagist.com/php-versions-stats-2018-2-edition/

And we also need to go ahead and drop unsupported PHP versions. A lot of libraries already dropped support for PHP 5 & 7.0. Which means, if we want to use new/update/security stuff from them, we'll also need to drop them.

We don't know when wallabag 2.4.0 will be released (in the coming weeks, months, years?). It depends on how free time we can dedicate to it.

Finally it's not because we are going to release wallabag 2.4.0 that we are going to stop fixing bug in 2.3.*. I guess we'll have a 2.3 branch and we'll push fixes on it from time to time.

Finally (again), there are still alternative that people on debian use a lot to get fresh version of PHP on their server https://deb.sury.org/

@nebulade

This comment has been minimized.

Copy link
Contributor

nebulade commented Nov 7, 2018

Is there anything I could help out here? Since we at Cloudron have updated postgres, our wallabag package had to be suspended for the time being until this is fixed.

To provide some more information, might be totally obvious, but I am not very familiar with wallabag nor Doctrine/DBAL, we do see the failure in a sql query at https://github.com/wallabag/wallabag/blob/master/src/Wallabag/CoreBundle/Doctrine/DBAL/Schema/CustomPostgreSqlSchemaManager.php#L26

15:07:40 - + /usr/local/bin/gosu www-data:www-data php bin/console doctrine:migrations:migrate --env prod --no-interaction
15:07:40 -                                                               
15:07:40 -                     Application Migrations                    
15:07:40 -                                                               
15:07:40 - 
15:07:40 - Migrating up to 20181029131313 from 20160401000000
15:07:40 - 
15:07:40 -   ++ migrating 20160410190541
15:07:40 - 
15:07:40 - Migration 20160410190541 failed during Execution. Error An exception occurred while executing 'SELECT min_value, increment_by FROM "entry_id_seq"':
15:07:40 - 
15:07:40 - SQLSTATE[42703]: Undefined column: 7 ERROR:  column "min_value" does not exist
15:07:40 - LINE 1: SELECT min_value, increment_by FROM "entry_id_seq"
15:07:40 -                ^
15:07:40 - 14:07:40 ERROR     [console] Error thrown while running command "doctrine:migrations:migrate --env prod --no-interaction". Message: "An exception occurred while executing 'SELECT min_value, increment_by FROM "entry_id_seq"':
15:07:40 - 
15:07:40 - SQLSTATE[42703]: Undefined column: 7 ERROR:  column "min_value" does not exist
15:07:40 - LINE 1: SELECT min_value, increment_by FROM "entry_id_seq"
15:07:40 -                ^" ["exception" => Doctrine\DBAL\Exception\InvalidFieldNameException { 鈥,"command" => "doctrine:migrations:migrate --env prod --no-interaction","message" => """  An exception occurred while executing 'SELECT min_value, increment_by FROM "entry_id_seq"':\n  \n  SQLSTATE[42703]: Undefined column: 7 ERROR:  column "min_value" does not exist\n  LINE 1: SELECT min_value, increment_by FROM "entry_id_seq"\n                 ^  """]
15:07:40 - 

As said I am a bit clueless here, but I cannot find any location where such a column would be created in the first place. Also the file header suggests, that it is indeed there to support postgres 10, so maybe this migration step is simply not finished yet? If so, is there anything I could help?

Edit: forgot to mention, that I have used this pull-request branch.

@j0k3r

This comment has been minimized.

Copy link
Member Author

j0k3r commented Nov 7, 2018

@nebulade I think once this PR will be merged, the official support for postgres 10 will be ok as DBAL will be updated to 2.6.0. Before that, I think you can't help us. But thanks for asking 馃

@Kdecherf

This comment has been minimized.

Copy link
Contributor

Kdecherf commented Nov 7, 2018

Hello @nebulade,

This PR does not directly fix the issue with PostgreSQL >= 10 but we need it before attempting needed dependency bump, see #3739 #3739 (comment)

@nebulade

This comment has been minimized.

Copy link
Contributor

nebulade commented Nov 7, 2018

Thanks for the clarification. Let me know if there is anything to help testing at least.

@childnode

This comment has been minimized.

Copy link

childnode commented Nov 7, 2018

But some PHP versions are going to be dropped in the coming months: PHP 5.6 & 7.0: http://php.net/supported-versions.php

good hint .. I was not that informed in the present past about the php lifecycle.
ok, then perhaps we should just head them to a migration hint when releasing this change to e.g. dockered environment https://doc.wallabag.org/en/admin/installation/installation.html#installation-with-docker backing up https://doc.wallabag.org/en/admin/backup.html and restore?!

j0k3r added some commits Oct 24, 2018

Fix RulerZBundle
People should really follow semver and provide UPGRADE file when they
provide a library ...

@j0k3r j0k3r force-pushed the dropping-php5 branch from a523f1c to 8d05443 Nov 25, 2018

Show resolved Hide resolved composer.json
@tcitworld

This comment has been minimized.

Copy link
Member

tcitworld commented Nov 26, 2018

I would have added php7.3 support. Do you want it here or in another PR ?

@j0k3r

This comment has been minimized.

Copy link
Member Author

j0k3r commented Nov 26, 2018

@tcitworld this is planned.
I'm currently upgrading all bundle to latest version (and it tooks time to update our code to be compatible with them).

@j0k3r j0k3r force-pushed the dropping-php5 branch 2 times, most recently from 0d0d0f4 to 2774ed3 Nov 28, 2018

j0k3r added some commits Oct 24, 2018

Jump to unrelease predis
To fix deprecated message regarding `each()`

j0k3r added some commits Nov 26, 2018

Remove custom Postgres class
Because PG > 10 is now supported by DBAL >= 2.6.0
CS
Force PHPUnit
Looks like "dama/doctrine-test-bundle" isn't compatible with PHPUnit 5.7
(required automatically by PHPUnit Bridge)
Use our own fork for CraueConfigBundle
Mostly to fix utf8mb4 issue
Fix utf8mb4 on vendor tables
When creating the schema for test these tables use default length for
string: 255. Which fail when using utf8mb4.

> Syntax error or access violation: 1071 Specified key was too long; max key length is 767 bytes

Also move the `setKeepStaticConnections` in before and after class to
avoid:

> SAVEPOINT DOCTRINE2_SAVEPOINT_2 does not exist

See https://github.com/dmaicher/doctrine-test-bundle#troubleshooting

@j0k3r j0k3r force-pushed the dropping-php5 branch from 2774ed3 to 877787e Nov 28, 2018

@j0k3r j0k3r requested review from tcitworld and Kdecherf Nov 28, 2018

@@ -1,4 +1,4 @@
Copyright (c) 2013-2017 Nicolas L艙uillet
Copyright (c) 2013-current Nicolas L艙uillet

This comment has been minimized.

Copy link
@tcitworld

tcitworld Nov 29, 2018

Member

Not sure if this is allowed.

This comment has been minimized.

Copy link
@Kdecherf

Kdecherf Nov 29, 2018

Contributor

Some random opinions say that the year of first publication may be enough, don't know if this can apply to source code.

@@ -11,8 +11,6 @@ parameters:
# database_password: %env.database_password%

database_driver: pdo_mysql
database_driver_class: ~

This comment has been minimized.

Copy link
@tcitworld

tcitworld Nov 29, 2018

Member

Don't forget to put in the release notes advice to admins to delete this line.

This comment has been minimized.

Copy link
@j0k3r

j0k3r Nov 29, 2018

Author Member

Yup.
But it鈥檚 not really a problem to let that value une the file as it鈥檚 not used anymore.
I鈥檒l also update the doc

@Kdecherf
Copy link
Contributor

Kdecherf left a comment

LGTM 馃憤

@j0k3r j0k3r merged commit 39502b4 into 2.4 Nov 29, 2018

3 checks passed

Scrutinizer 22 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

2.4 automation moved this from To Do to Done Nov 29, 2018

@j0k3r j0k3r deleted the dropping-php5 branch Nov 29, 2018

@nebulade

This comment has been minimized.

Copy link
Contributor

nebulade commented Nov 29, 2018

Thanks for pushing this forward! While trying to test this, I keep on getting the following composer install error:

Loading composer repositories with package information
Failed to clone the git@github.com:Daniel-KM/PHPePub.git repository, try running in interactive mode so that you can enter your GitHub credentials

In Git.php line 325:
                                                                               
  Failed to execute git clone --mirror 'git@github.com:Daniel-KM/PHPePub.git'  
   '/root/.composer/cache/vcs/git-github.com-Daniel-KM-PHPePub.git/'           
                                                                               

install [--prefer-source] [--prefer-dist] [--dry-run] [--dev] [--no-dev] [--no-custom-installers] [--no-autoloader] [--no-scripts] [--no-progress] [--no-suggest] [-v|vv|vvv|--verbose] [-o|--optimize-autoloader] [-a|--classmap-authoritative] [--apcu-autoloader] [--ignore-platform-reqs] [--] [<packages>]...

This looks like a new dependency, which needs to be built from git? I am not very familiar with composer, so maybe this is just an issue on my end, in which case, please ignore.

@tcitworld

This comment has been minimized.

Copy link
Member

tcitworld commented Nov 29, 2018

Do you have git installed聽?

@nebulade

This comment has been minimized.

Copy link
Contributor

nebulade commented Nov 29, 2018

Yes git version 2.17.1 and Composer 1.6.3 2018-01-31 16:28:17

@Kdecherf

This comment has been minimized.

Copy link
Contributor

Kdecherf commented Nov 29, 2018

You must have a ssh key linked to your github account when cloning the Daniel-KM/PHPePub.git repository from composer.

If you can't or don't want to link your github account to the system account (root in this case which is not a good idea), you could use https://github.com/Daniel-KM/PHPePub.git instead.

@nebulade

This comment has been minimized.

Copy link
Contributor

nebulade commented Nov 29, 2018

I do not have patched the wallabag code here, this is just calling composer install like before these patches. Maybe I am using composer wrongly with the new dependencies?

@gramakri

This comment has been minimized.

Copy link

gramakri commented Nov 29, 2018

@nebulade Maybe we can set github-protocols as suggested by https://stackoverflow.com/questions/18638395/how-to-force-composer-to-use-https-instead-of-git . Looks like composer has some built-in support for github and we prefer ssh clone by default.

@j0k3r

This comment has been minimized.

Copy link
Member Author

j0k3r commented Nov 30, 2018

What did you get when you manually perform that command git clone --mirror 'git@github.com:Daniel-KM/PHPePub.git' '/root/.composer/cache/vcs/git-github.com-Daniel-KM-PHPePub.git/'?
Also, did you try the config global command from the SO first answer? Does it work?

@nebulade

This comment has been minimized.

Copy link
Contributor

nebulade commented Dec 2, 2018

None of those settings have worked, however it is clearly a composer relate issue itself, so not really related to this thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.