Remove HHVM support #22758

fabpot · 2017-05-18T17:44:11Z

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

nicolas-grekas · 2017-05-18T17:48:14Z

src/Symfony/Component/VarDumper/Caster/ReflectionCaster.php

@@ -36,7 +36,7 @@ public static function castClosure(\Closure $c, array $a, Stub $stub, $isNested,
        $prefix = Caster::PREFIX_VIRTUAL;
        $c = new \ReflectionFunction($c);

-        $stub->class = 'Closure'; // HHVM generates unique class names for closures
+        $stub->class = 'Closure';


The line itself can be removed

nicolas-grekas · 2017-05-18T17:48:55Z

src/Symfony/Component/VarDumper/Tests/Dumper/CliDumperTest.php

@@ -46,7 +46,7 @@ public function testGet()
        $intMax = PHP_INT_MAX;
        $res = (int) $var['res'];

-        $r = defined('HHVM_VERSION') ? '' : '#%d';
+        $r = '#%d';


Can be inlined

stof · 2017-05-18T17:49:30Z

src/Symfony/Component/VarDumper/Tests/Dumper/CliDumperTest.php

@@ -46,7 +46,7 @@ public function testGet()
        $intMax = PHP_INT_MAX;
        $res = (int) $var['res'];

-        $r = defined('HHVM_VERSION') ? '' : '#%d';
+        $r = '#%d';


I would put it directly in the string instead of using a variable, as it does not change anymore

stof · 2017-05-18T17:49:45Z

src/Symfony/Component/VarDumper/Tests/Dumper/CliDumperTest.php

@@ -325,7 +321,7 @@ public function testThrowingCaster()
        $dumper->dump($data, $out);
        $out = stream_get_contents($out, -1, 0);

-        $r = defined('HHVM_VERSION') ? '' : '#%d';
+        $r = '#%d';


stof · 2017-05-18T17:49:50Z

src/Symfony/Component/VarDumper/Tests/Dumper/CliDumperTest.php

@@ -384,7 +380,7 @@ public function testRefsInProperties()
        $data = $cloner->cloneVar($var);
        $out = $dumper->dump($data, true);

-        $r = defined('HHVM_VERSION') ? '' : '#%d';
+        $r = '#%d';


stof · 2017-05-18T17:49:57Z

src/Symfony/Component/VarDumper/Tests/Dumper/HtmlDumperTest.php

@@ -47,7 +47,7 @@ public function testGet()
        $dumpId = $dumpId[0];
        $res = (int) $var['res'];

-        $r = defined('HHVM_VERSION') ? '' : '<a class=sf-dump-ref>#%d</a>';
+        $r = '<a class=sf-dump-ref>#%d</a>';


ro0NL · 2017-05-19T06:21:56Z

There are many more occurrences right? Or is it work in progress :)

sstok · 2017-05-19T14:42:37Z

src/Symfony/Component/Cache/Adapter/PhpArrayAdapter.php

-        // Shared memory is available in PHP 7.0+ with OPCache enabled and in HHVM
-        if ((PHP_VERSION_ID >= 70000 && ini_get('opcache.enable')) || defined('HHVM_VERSION')) {
+        // Shared memory is available in PHP 7.0+ with OPCache enabled
+        if (PHP_VERSION_ID >= 70000 && ini_get('opcache.enable')) {


PHP Version check can also be removed as PHP 7.1 is now the minimum.

not in this PR though, to make the changelog and the patch consistent

derrabus · 2017-05-21T15:50:06Z

src/Symfony/Component/ClassLoader/Tests/ClassCollectionLoaderTest.php

 EOF
-            .$strictTypes,


You should keep the comma.

flip111 · 2017-05-22T11:29:29Z

Already many people switch their infrastructure to HHVM this PR will complicate working with symfony for them.

xabbuh · 2017-05-22T11:32:47Z

Are the really people switching from PHP 7 to HHVM (or from PHP 5 to HHVM instead of PHP 7)?

javiereguiluz · 2017-05-22T11:33:41Z

@flip111 I'm sorry but the evidence we have (including Composer's own stats abut PHP usage) shows that almost nobody is using HHVM.

flip111 · 2017-05-22T11:41:49Z

People were switching to HHVM before PHP 7 was a thing. Since removing functionality (support for an alternative platform) is a big thing i had at least expected to see something positive here on the tradeoff.

nicolas-grekas · 2017-05-22T11:45:13Z

@flip111 see also #22765. The thing is, since things do not happen by magic, HHVM support needs someone to care about it. Here, nobody cared nor made it to work on a PHP 7.1 code base. Unless you want to do the job, it looks like nobody did in the last months, if not years.

flip111 · 2017-05-22T11:49:40Z

@nicolas-grekas fair enough, would have been nice to have in the PR description something like "hhvm support was not properly maintained for months, removing to reduce the burdon of maintaining this code. [User statistics](link) show that hhvm was hardly used.". This is not obvious to know before i asked about it.

derrabus · 2017-05-22T12:41:18Z

So, if HHVM support is being removed from master, that means it'll stay around for Symfony 3.4 LTS which is going to be maintained until November 2020, am I right? So, I'd say HHVM/Symfony projects are safe for another 3.5 years.

Within those 3.5 years, those projects could be migrated away from HHVM (or Symfony if you like) or HHVM maybe manages to get their compatibility issues under control.

And Symfony is going to be your least problem. Other libraries that go php-7-only will be running into similar trouble and most of the have shorter maintenance periods than Symfony does.

Anyway, maybe the drop of HHVM should be announced in a blog article with some explanation to avoid confusion.

nicolas-grekas · 2017-05-22T12:42:47Z

@derrabus 💯

stof · 2017-05-22T13:12:07Z

I agree with the fact we should write a blog post about this /cc @javiereguiluz

Tobion · 2017-05-22T13:47:27Z

src/Symfony/Bundle/WebServerBundle/Command/ServerCommand.php

@@ -20,11 +20,4 @@
 */
 abstract class ServerCommand extends Command


Looks like this class should be tagged internal

Shouldn't it be removed (and marked deprecated in 3.x) instead? It doesn't really serve a purpose anymore, does it?

could still me marked internal in 3.3 right? and so removable in 4.0 :)

it has been added in 3.3, so we should act NOW

my point :) deprecation way seems tedious.

fabpot · 2017-05-23T05:55:12Z

I'm going to write a blog post about this.

fixed

fabpot · 2017-05-23T07:35:01Z

See http://symfony.com/blog/symfony-4-end-of-hhvm-support for the blog post

…s-grekas) This PR was merged into the 3.3 branch. Discussion ---------- [WebServerBundle] Mark ServerCommand as internal | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22758 (comment) | License | MIT | Doc PR | - Commits ------- c91d195 [WebServerBundle] Mark ServerCommand as internal

This PR was squashed before being merged into the 4.0-dev branch (closes #22758). Discussion ---------- Remove HHVM support | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes-ish (for HHVM users) | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Commits ------- 2125437 [WebServerBundle] removed obsolete ServerCommand abstract class ad1f35f removed HHVM support

This PR removes HHVM compatiblity following symfony#22758 It also removes some unneeded comments

This PR was squashed before being merged into the 4.0-dev branch (closes #23216). Discussion ---------- Remove HHVM support (second edition) | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none | License | MIT | Doc PR | not needed This PR removes HHVM compatiblity following #22758 as some things were missing. It also removes some useless comments. Commits ------- 471b84c Remove HHVM support (second edition)

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

fabpot force-pushed the hhvm-support-removal branch from 353b933 to 6fd2053 Compare May 18, 2017 17:46

nicolas-grekas reviewed May 18, 2017

View reviewed changes

stof previously requested changes May 18, 2017

View reviewed changes

fabpot force-pushed the hhvm-support-removal branch 4 times, most recently from eef1f42 to 1972250 Compare May 18, 2017 18:13

nicolas-grekas added this to the 4.0 milestone May 19, 2017

fabpot force-pushed the hhvm-support-removal branch from 1972250 to 3281ccb Compare May 19, 2017 12:34

sstok reviewed May 19, 2017

View reviewed changes

derrabus reviewed May 21, 2017

View reviewed changes

src/Symfony/Component/ClassLoader/Tests/ClassCollectionLoaderTest.php Outdated

EOF

.$strictTypes,

Copy link

Member

derrabus May 21, 2017

Choose a reason for hiding this comment

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

You should keep the comma.

Tobion reviewed May 22, 2017

View reviewed changes

fabpot force-pushed the hhvm-support-removal branch from 26045cb to bdf9da0 Compare May 23, 2017 05:52

nicolas-grekas mentioned this pull request May 23, 2017

[WebServerBundle] Mark ServerCommand as internal #22869

Merged

fabpot force-pushed the hhvm-support-removal branch from bdf9da0 to 02588ad Compare May 23, 2017 10:05

fabpot force-pushed the hhvm-support-removal branch from bdf9da0 to 35569df Compare June 1, 2017 21:06

fabpot added 2 commits June 1, 2017 15:17

removed HHVM support

ad1f35f

[WebServerBundle] removed obsolete ServerCommand abstract class

2125437

fabpot force-pushed the hhvm-support-removal branch from 35569df to 2125437 Compare June 1, 2017 22:18

fabpot merged commit 2125437 into symfony:master Jun 1, 2017

thekid mentioned this pull request Jun 10, 2017

Restore HHVM support xp-framework/core#176

Closed

Nek- added a commit to Nek-/symfony that referenced this pull request Jun 17, 2017

Remove HHVM compatibility

c18f9bd

This PR removes HHVM compatiblity following symfony#22758 It also removes some unneeded comments

Nek- mentioned this pull request Jun 17, 2017

Remove HHVM support (second edition) #23216

Closed

Nek- added a commit to Nek-/symfony that referenced this pull request Jun 17, 2017

Remove HHVM compatibility

09ad255

This PR removes HHVM compatiblity following symfony#22758 It also removes some unneeded comments

greg0ire mentioned this pull request Jul 2, 2017

use trusty for hhvm build sonata-project/dev-kit#250

Merged

fabpot deleted the hhvm-support-removal branch September 11, 2017 18:18

fabpot mentioned this pull request Oct 19, 2017

Release v4.0.0-BETA1 #24612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove HHVM support #22758

Remove HHVM support #22758

fabpot commented May 18, 2017

nicolas-grekas May 18, 2017

nicolas-grekas May 18, 2017

stof May 18, 2017

stof May 18, 2017

stof May 18, 2017

stof May 18, 2017

ro0NL commented May 19, 2017

sstok May 19, 2017

nicolas-grekas May 19, 2017

derrabus May 21, 2017

flip111 commented May 22, 2017

xabbuh commented May 22, 2017

javiereguiluz commented May 22, 2017

flip111 commented May 22, 2017

nicolas-grekas commented May 22, 2017 •

edited

Loading

flip111 commented May 22, 2017 •

edited

Loading

derrabus commented May 22, 2017

nicolas-grekas commented May 22, 2017

stof commented May 22, 2017

Tobion May 22, 2017

derrabus May 22, 2017

ro0NL May 22, 2017

nicolas-grekas May 22, 2017

ro0NL May 22, 2017

fabpot May 23, 2017

fabpot commented May 23, 2017

fabpot commented May 23, 2017

		@@ -20,11 +20,4 @@
		*/
		abstract class ServerCommand extends Command

Remove HHVM support #22758

Remove HHVM support #22758

Conversation

fabpot 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

ro0NL commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip111 commented May 22, 2017

xabbuh commented May 22, 2017

javiereguiluz commented May 22, 2017

flip111 commented May 22, 2017

nicolas-grekas commented May 22, 2017 • edited Loading

flip111 commented May 22, 2017 • edited Loading

derrabus commented May 22, 2017

nicolas-grekas commented May 22, 2017

stof commented May 22, 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

fabpot commented May 23, 2017

fabpot commented May 23, 2017

nicolas-grekas commented May 22, 2017 •

edited

Loading

flip111 commented May 22, 2017 •

edited

Loading