Bump minimum version to PHP 7.1 for Symfony 4 #22733

fabpot · 2017-05-17T18:43:05Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes-ish?
BC breaks?	yes-ish?
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	n/a

stof · 2017-05-17T19:02:08Z

.travis.yml

-        - php: 5.5
-        - php: 5.6
-        - php: 7.0
+        - php: 7.1.0


we should have a test running the fullstack testsuite on latest 7.1 too IMO

We have one, like in the previous branches: the deps=low line does it

deps=low is a per-component build, not a fullstack build. It does not test the requirements of the fullstack

Unless I misunderstood what you meant, the line you're asking for doesn't exist on the SF3 lines. Thus we should not add it since it has never been required.

stof · 2017-05-17T19:03:30Z

appveyor.yml

@@ -59,8 +57,6 @@ test_script:
    - cd c:\php && 7z x php-7.1.3-Win32-VC14-x64.zip -y >nul && copy /Y php.ini-min php.ini
    - cd c:\projects\symfony
    - php phpunit src\Symfony --exclude-group benchmark,intl-data || SET X=!errorlevel!
-    - cd c:\php && 7z x php-5.5.9-nts-Win32-VC11-x86.zip -y >nul && copy /Y php.ini-min php.ini
-    - cd c:\projects\symfony
    - SET SYMFONY_PHPUNIT_SKIPPED_TESTS=phpunit.skipped
    - php phpunit src\Symfony --exclude-group benchmark,intl-data || SET X=!errorlevel!


this line should be removed, as it was the one running tests on 5.5.9. there is no need to run tests a second time on 7.1.3 without changing the php.ini (skipped tests will stay the same)

stof · 2017-05-17T19:05:42Z

@fabpot when bumping the version to 4.0, you forgot to update all inter-component requirements to allow them to use the 4.0 version of other components in their own 4.0 versions

nicolas-grekas · 2017-05-17T19:13:15Z

Looks like this requires us to drop HHVM, impossible to make it run on the CI. I'm fine with that personally.

fabpot · 2017-05-18T05:44:47Z

Poll here: https://twitter.com/fabpot/status/865080534669635584

javiereguiluz · 2017-05-18T06:20:07Z

In addition to the poll, according to the latest global PHP stats, HHVM is used by 0.36% of PHP developers. The number is practically zero ... but maybe there are some high-profile clients there (Facebook is using it, Slack was thinking about using it, ...)

Seldaek · 2017-05-18T06:44:12Z

@javiereguiluz please don't forget that the stats are about number of composer installs.. That doesn't translate 1:1 to users, as some may do more installs than others. I think it's mostly interesting to see trends over time as exact point-in-time numbers can't be fully trusted.

stof · 2017-05-18T07:27:30Z

@nicolas-grekas HHVM has 2 modes: PHP 5 mode and PHP 7 mode. IIRC, it runs in PHP 5 mode by default. We should add hhvm.php7.all = 1 in the HHVM INI file.

nicolas-grekas · 2017-05-18T07:29:18Z

I just tried again this mode, same failure. We'd need a php 7.1 mode. There is just no way currently to make hhvm run on a 7.1 code base. HHVM advertises itself as 70099 so even Composer cant.
Just dropping for now.

stof · 2017-05-18T07:45:44Z

Have you tried using the latest HHVM rather than 3.18 ? they may have updated their PHP 7 mode to expose it as 7.1 later

nicolas-grekas · 2017-05-18T07:47:46Z

Yep, 3.19 locally, same result

stof · 2017-05-18T07:46:26Z

.travis.yml

      # Load fixtures
      if [[ ! $skip ]]; then
+          sleep 5


why sleeping ?

Previously we did not require it because we had some extensions to compile, which gave some time for the ldap sever to start in the background. But now we have to sleep a bit to let it start.

nicolas-grekas · 2017-05-18T08:33:24Z

composer.json

@@ -16,7 +16,7 @@
        }
    ],
    "require": {
-        "php": ">=5.5.9",
+        "php": "^7.1.0",


shouldn't we keep the ">=" operator?

Well, this constraint forbids using the existing version with PHP 8. I think it is fine. I'm almost sure that the current code will not work with PHP 8 (PHP major versions will break BC for sure if we look at the existing releases where even minor versions have a hard time preserving BC fully). This way, when PHP 8 starts to come out, we can update our requirements once we start testing it with PHP 7

This way exclude PHP8, IMO it's a good thing and avoid future PR to exclude it and then re-enable PHP8 after BC breaks corrected.

nicolas-grekas · 2017-05-18T09:22:52Z

.travis.yml

@@ -83,11 +77,11 @@ before_install:
      echo extension = ldap.so >> $INI
      echo extension = redis.so >> $INI
      echo extension = memcached.so >> $INI
-      [[ $PHP = 5.* ]] && echo extension = mongo.so >> $INI
-      [[ $PHP = 5.* ]] && echo extension = memcache.so >> $INI
+      #echo extension = mongodb.so >> $INI


the test suite does not pass yet with mongodb - but this is not master specific so should be handled on a lower branch

xabbuh · 2017-05-18T13:43:28Z

src/Symfony/Bundle/FrameworkBundle/composer.json

-        "symfony/http-foundation": "~3.3|~4.0.0",
-        "symfony/http-kernel": "~3.3|~4.0.0",
+        "symfony/cache": "~3.4|~4.0",
+        "symfony/class-loader": "~3.4|~4.0",


The ClassLoader component won't have a 4.0 release.

yeah, but the requirement will be dropped entirely when removing deprecated code anyway (only the BC layer requires it), so it is fine for now

xabbuh · 2017-05-18T13:43:43Z

src/Symfony/Component/HttpKernel/composer.json

-        "symfony/translation": "~2.8|~3.0|~4.0.0",
-        "symfony/var-dumper": "~3.3|~4.0.0",
+        "symfony/browser-kit": "~3.4|~4.0",
+        "symfony/class-loader": "~3.4|~4.0",


The ClassLoader component won't have a 4.0 release.

let's do that in the next PR removing class loader, here it's a batch change

fair enough

xabbuh · 2017-05-18T13:44:06Z

src/Symfony/Component/HttpKernel/composer.json

    },
    "suggest": {
        "symfony/browser-kit": "",
+        "symfony/class-loader": "",


should be removed

nicolas-grekas · 2017-05-18T14:02:18Z

Thank you @fabpot.

nicolas-grekas · 2017-05-18T14:02:30Z

;)

… dunglas, nicolas-grekas) This PR was merged into the 4.0-dev branch. Discussion ---------- Bump minimum version to PHP 7.1 for Symfony 4 | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes-ish? | BC breaks? | yes-ish? | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Commits ------- 4758c2c Tweak travis and appveyor for Symfony 4 6633c8b Allow individual bridges, bundles and components to be used with 4.0 c850733 bumped minimum version to PHP 7.1

mofarrell · 2017-05-18T18:43:26Z

Is this just a version number issue? HHVM can change what version number it gives back. Are there some PHP7 features we haven't implemented yet that are making it burdensome for Symfony to support HHVM?

nicolas-grekas · 2017-05-18T18:57:49Z

That's not the only issue because even with the branches that support php5.5 thus hhvm, once you enable the php7 mode the test suite fails to just run, with strange type issues being reported.
It's quite easy to reproduce: just try running the ./phpunit script in this mode on e.g. 3.2.

mofarrell · 2017-05-18T19:27:17Z

We have an suboption to PHP7 that can disable the strict type checks. The strict type checks are broken in some cases, and we have that on our radar. It happens to not be trivial to fix, and also currently not at the top of our priority list.
The option is hhvm.php7.scalar_types=0

This PR was squashed before being merged into the 4.0-dev branch (closes #22820). Discussion ---------- Remove PHP < 7.1.3 code | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no  | Tests pass? | yes | Fixed tickets | #22733 | License | MIT | Doc PR | N/A Commits ------- 7091fb4 Remove PHP < 7.1.3 code

SenseException · 2017-05-22T15:22:18Z

Will Symfony 4 use scalar type hints and return types? This would be a break to the 3.x versions, but with a major release the best time to introduce them.

stof · 2017-05-22T15:35:00Z

@SenseException we will use them in new APIs. But we won't add them in existing interfaces, as this would be a BC break without a progressive upgrade path, and so the drawbacks are much bigger than the benefits for the community.

Btw, the master branch already contains a scalar typehint in at least one place (the last argument in

symfony/src/Symfony/Component/Security/Http/Logout/LogoutUrlGenerator.php

Line 51 in d6bdd23

    
           public function registerListener($key, $logoutPath, $csrfTokenId, $csrfParameter, CsrfTokenManagerInterface $csrfTokenManager = null, string $context = null)

)

SenseException · 2017-05-22T17:03:42Z

@stof I'm aware that this would be a bold move, but your argument also fits to a future Symfony 5, 6, 7 and so on. Implementing an old Symfony 4 interface forces a user to omit type hints even when it is a new project that never was on version 3 before.

But it seems that a return type can be added to the class implementing an interface that misses that type: https://3v4l.org/BHlVW.

sstok · 2017-05-28T08:28:16Z

@SenseException yes you can add return types to implementations, even when the interface doesn't require them. But doing this would be something for Symfony 5 so developers can first upgrade to Symfony 4, add the return-types, and then when Symfony 5 is released they can safely upgrade.

SenseException · 2017-05-29T08:54:02Z

So Symfony 5 is going to have the type hints and return types? I just want to read when Symfony and its components arr going to use modern (2017) PHP.

dunglas · 2017-05-29T08:56:33Z

We'll use type hints and return types for the new code. It's not possible without a BC break for the old one (and it has very little interest because, as pointed out by other people, you can use type hints in your code and our code base is well tested).

javiereguiluz · 2017-05-29T08:58:17Z

@SenseException in addition to what Kévin told you, we can only use PHP 7 features for new code added to Symfony. For the existing code, we can't use PHP 7 until November 2021 (as explained in this comment: #22862 (comment))

SenseException · 2017-06-08T13:04:13Z

@javiereguiluz Thank you for pointing me to the comment and the answer to my question.

Aliance · 2017-10-19T07:22:35Z

src/Symfony/Component/HttpKernel/Kernel.php

@@ -63,7 +63,7 @@

    const VERSION = '4.0.0-DEV';
    const VERSION_ID = 40000;
-    const MAJOR_VERSION = 4;
+    const MAJOR_VERSION = 0;


Why did you change this?

That seems like a bug in the release script 😛

It was already fixed in ce519b6

This PR was squashed before being merged into the 4.0-dev branch (closes #22820). Discussion ---------- Remove PHP < 7.1.3 code | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no  | Tests pass? | yes | Fixed tickets | symfony/symfony#22733 | License | MIT | Doc PR | N/A Commits ------- 54adaf8 Remove PHP < 7.1.3 code

carsonbot added Status: Needs Review Feature BC Break labels May 17, 2017

fabpot force-pushed the 4.0-PHP-bump branch 5 times, most recently from 054280d to 0c3616c Compare May 17, 2017 19:03

stof requested changes May 17, 2017

View reviewed changes

nicolas-grekas force-pushed the 4.0-PHP-bump branch from cc41e57 to 224b043 Compare May 18, 2017 07:45

nicolas-grekas force-pushed the 4.0-PHP-bump branch from 224b043 to c0e4f53 Compare May 18, 2017 07:54

stof reviewed May 18, 2017

View reviewed changes

nicolas-grekas force-pushed the 4.0-PHP-bump branch from c0e4f53 to fd56941 Compare May 18, 2017 08:31

nicolas-grekas reviewed May 18, 2017

View reviewed changes

nicolas-grekas force-pushed the 4.0-PHP-bump branch 2 times, most recently from fb64a47 to b930c86 Compare May 18, 2017 09:19

nicolas-grekas reviewed May 18, 2017

View reviewed changes

nicolas-grekas force-pushed the 4.0-PHP-bump branch 4 times, most recently from 1482630 to 809736f Compare May 18, 2017 11:40

nicolas-grekas force-pushed the 4.0-PHP-bump branch from fc893b2 to 47a3b51 Compare May 18, 2017 13:36

xabbuh requested changes May 18, 2017

View reviewed changes

Tweak travis and appveyor for Symfony 4

4758c2c

nicolas-grekas force-pushed the 4.0-PHP-bump branch from 47a3b51 to 4758c2c Compare May 18, 2017 13:50

xabbuh approved these changes May 18, 2017

View reviewed changes

dunglas approved these changes May 18, 2017

View reviewed changes

stof approved these changes May 18, 2017

View reviewed changes

nicolas-grekas merged commit 4758c2c into symfony:master May 18, 2017

robfrawley mentioned this pull request May 20, 2017

HHVM and PHP7 differ on type annotations on internal functions (rtrim in particular) facebook/hhvm#7198

Closed

ogizanagi mentioned this pull request May 21, 2017

Remove PHP < 7.1.3 code #22820

Closed

keradus mentioned this pull request May 22, 2017

Use PHP 7.1 syntax #22862

Closed

robfrawley mentioned this pull request May 23, 2017

[RFC] What version of PHP are you running? What should we require? liip/LiipImagineBundle#873

Closed

fabpot mentioned this pull request Oct 19, 2017

Release v4.0.0-BETA1 #24612

Merged

Aliance reviewed Oct 19, 2017

View reviewed changes

Bump minimum version to PHP 7.1 for Symfony 4 #22733

Bump minimum version to PHP 7.1 for Symfony 4 #22733

Conversation

fabpot commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented May 17, 2017

nicolas-grekas commented May 17, 2017

fabpot commented May 18, 2017

javiereguiluz commented May 18, 2017

Seldaek commented May 18, 2017

stof commented May 18, 2017

nicolas-grekas commented May 18, 2017 • edited Loading

stof commented May 18, 2017

nicolas-grekas commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented May 18, 2017

nicolas-grekas commented May 18, 2017

mofarrell commented May 18, 2017

nicolas-grekas commented May 18, 2017 • edited Loading

mofarrell commented May 18, 2017

SenseException commented May 22, 2017

stof commented May 22, 2017

SenseException commented May 22, 2017

sstok commented May 28, 2017

SenseException commented May 29, 2017

dunglas commented May 29, 2017 • edited Loading

javiereguiluz commented May 29, 2017

SenseException commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas May 18, 2017 •

edited

Loading

nicolas-grekas commented May 18, 2017 •

edited

Loading

nicolas-grekas commented May 18, 2017 •

edited

Loading

dunglas commented May 29, 2017 •

edited

Loading