8000 Remove HHVM support (second edition) by Nek- · Pull Request #23216 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Remove HHVM support (second edition) #23216

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Remove HHVM support (second edition) #23216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09ad255
Remove HHVM compatibility
Nek- Jun 17, 2017
a5bcfd6
Remove last parts of HHVM in ErrorHandler
Nek- Jun 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Symfony/Component/Cache/Adapter/PhpArrayAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ function ($key, $value, $isHit) {
*/
public static function create($file, CacheItemPoolInterface $fallbackPool)
{
// Shared memory is available in PHP 7.0+ with OPCache enabled and in HHVM
// Shared memory is available in PHP 7.0+ with OPCache enabled
if (ini_get('opcache.enable')) {
if (!$fallbackPool instanceof AdapterInterface) {
$fallbackPool = new ProxyAdapter($fallbackPool);
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Cache/Simple/PhpArrayCache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public function __construct($file, CacheInterface $fallbackPool)
*/
public static function create($file, CacheInterface $fallbackPool)
{
// Shared memory is available in PHP 7.0+ with OPCache enabled and in HHVM
// Shared memory is available in PHP 7.0+ with OPCache enabled
if (ini_get('opcache.enable')) {
return new static($file, $fallbackPool);
}
Expand Down
33 changes: 8 additions & 25 deletions src/Symfony/Component/Debug/ErrorHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,10 +382,8 @@ public function handleError($type, $message, $file, $line)

if (4 < $numArgs = func_num_args()) {
$context = $scope ? (func_get_arg(4) ?: array()) : array();
$backtrace = 5 < $numArgs ? func_get_arg(5) : null; // defined on HHVM
} else {
$context = array();
$backtrace = null;
}

if (isset($context['GLOBALS']) && $scope) {
Expand All @@ -394,15 +392,6 @@ public function handleError($type, $message, $file, $line)
$context = $e;
}

if (null !== $backtrace && $type & E_ERROR) {
// E_ERROR fatal errors are triggered on HHVM when
// hhvm.error_handling.call_user_handler_on_fatals=1
// which is the way to get their backtrace.
$this->handleFatalError(compact('type', 'message', 'file', 'line', 'backtrace'));

return true;
}

$logMessage = $this->levels[$type].': '.$message;

if (null !== self::$toStringException) {
Expand Down Expand Up @@ -436,11 +425,12 @@ public function handleError($type, $message, $file, $line)

// Clean the trace by removing function arguments and the first frames added by the error handler itself.
if ($throw || $this->tracedErrors & $type) {
$backtrace = $backtrace ?: $errorAsException->getTrace();
$backtrace = $errorAsException->getTrace();
$lightTrace = $this->cleanTrace($backtrace, $type, $file, $line, $throw);
$this->traceReflector->setValue($errorAsException, $lightTrace);
} else {
$this->traceReflector->setValue($errorAsException, array());
$backtrace = array();
}
}

Copy link
Member

Choose a reason for hiding this comment

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

there are a few more HHVM cases below this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't removed them because the code is pretty much tricky and I'm not sure what it's used for so I didn't remove anything. ^^'

Copy link
Member

Choose a reason for hiding this comment

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

I'd say

diff --git a/src/Symfony/Component/Debug/ErrorHandler.php b/src/Symfony/Component/Debug/ErrorHandler.php
index 6fb2c94..ddcf193 100644
--- a/src/Symfony/Component/Debug/ErrorHandler.php
+++ b/src/Symfony/Component/Debug/ErrorHandler.php
@@ -444,32 +444,25 @@ class ErrorHandler
                         && ('trigger_error' === $backtrace[$i - 1]['function'] || 'user_error' === $backtrace[$i - 1]['function'])
                     ) {
                         // Here, we know trigger_error() has been called from __toString().
-                        // HHVM is fine with throwing from __toString() but PHP triggers a fatal error instead.
+                        // PHP triggers a fatal error when throwing from __toString().
                         // A small convention allows working around the limitation:
                         // given a caught $e exception in __toString(), quitting the method with
                         // `return trigger_error($e, E_USER_ERROR);` allows this error handler
                         // to make $e get through the __toString() barrier.
 
                         foreach ($context as $e) {
-                            if (($e instanceof \Exception || $e instanceof \Throwable) && $e->__toString() === $message) {
-                                if (1 === $i) {
-                                    // On HHVM
-                                    $errorAsException = $e;
-                                    break;
-                                }
+                            if ($e instanceof \Throwable && $e->__toString() === $message) {
                                 self::$toStringException = $e;
 
                                 return true;
                             }
                         }
 
-                        if (1 < $i) {
-                            // On PHP (not on HHVM), display the original error message instead of the default one.
-                            $this->handleException($errorAsException);
+                        // Display the original error message instead of the default one.
+                        $this->handleException($errorAsException);
 
-                            // Stop the process by giving back the error to the native handler.
-                            return false;
-                        }
+                        // Stop the process by giving back the error to the native handler.
+                        return false;
                     }
                 }
             }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

8000 Expand All @@ -454,32 +444,25 @@ public function handleError($type, $message, $file, $line)
&& ('trigger_error' === $backtrace[$i - 1]['function'] || 'user_error' === $backtrace[$i - 1]['function'])
) {
// Here, we know trigger_error() has been called from __toString().
// HHVM is fine with throwing from __toString() but PHP triggers a fatal error instead.
// PHP triggers a fatal error when throwing from __toString().
// A small convention allows working around the limitation:
// given a caught $e exception in __toString(), quitting the method with
// `return trigger_error($e, E_USER_ERROR);` allows this error handler
// to make $e get through the __toString() barrier.

foreach ($context as $e) {
if (($e instanceof \Exception || $e instanceof \Throwable) && $e->__toString() === $message) {
if (1 === $i) {
// On HHVM
$errorAsException = $e;
break;
}
if ($e instanceof \Throwable && $e->__toString() === $message) {
self::$toStringException = $e;

return true;
}
}

if (1 < $i) {
// On PHP (not on HHVM), display the original error message instead of the default one.
$this->handleException($errorAsException);
// Display the original error message instead of the default one.
$this->handleException($errorAsException);

// Stop the process by giving back the error to the native handler.
return false;
}
// Stop the process by giving back the error to the native handler.
return false;
}
}
}
Expand Down
7 changes: 1 addition & 6 deletions src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadataFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,7 @@ private function getType(\ReflectionParameter $parameter)
if (!$type = $parameter->getType()) {
return;
}
$typeName = $type->getName();
if ('array' === $typeName && !$type->isBuiltin()) {
// Special case for HHVM with variadics
return;
}

return $typeName;
return $type->getName();
}
}
1 change: 0 additions & 1 deletion src/Symfony/Component/Intl/Data/Bundle/Reader/IntlBundleReader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ public function read($path, $locale)
// Never enable fallback. We want to know if a bundle cannot be found
$bundle = new \ResourceBundle($locale, $path, false);
} catch (\Exception $e) {
// HHVM compatibility: constructor throws on invalid resource
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the try/catch be removed then? (same below)

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a config in Intl, that translate errors into exceptions. Before removing the try-catch, this has to be tested with that intl config intl.use_exceptions.

http://php.net/manual/de/intl.configuration.php

Copy link
Contributor Author
@Nek- Nek- Jun 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

PHP actually throws an exception. That's why I didn't remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

PHP 7.0 and above throws an exception without that config. That's true.

$bundle = null;
}

Expand Down
5 changes: 0 additions & 5 deletions src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,11 +177,6 @@ private function extractFromMutator($class, $property)
}
$type = $this->extractFromReflectionType($reflectionType);

// HHVM reports variadics with "array" but not builtin type hints
if (!$reflectionType->isBuiltin() && Type::BUILTIN_TYPE_ARRAY === $type->getBuiltinType()) {
return;
}

if (in_array($prefix, $this->arrayMutatorPrefixes)) {
$type = new Type(Type::BUILTIN_TYPE_ARRAY, false, null, true, new Type(Type::BUILTIN_TYPE_INT), $type);
}
Expand Down
1 change: 0 additions & 1 deletion src/Symfony/Component/Translation/Loader/IcuDatFileLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ public function load($resource, $locale, $domain = 'messages')
try {
$rb = new \ResourceBundle($locale, $resource);
} catch (\Exception $e) {
// HHVM compatibility: constructor throws on invalid resource
10000
Copy link
Member
@chalasr chalasr Jun 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

shouldn't the try-catch be removed instead? already pointed out

Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment for IntlBundleReader.php

$rb = null;
}

Expand Down
1 change: 0 additions & 1 deletion src/Symfony/Component/Translation/Loader/IcuResFileLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ public function load($resource, $locale, $domain = 'messages')
try {
$rb = new \ResourceBundle($locale, $resource);
} catch (\Exception $e) {
// HHVM compatibility: constructor throws on invalid resource
Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment for IntlBundleReader.php

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean this one? #23216 (comment) I answered.

$rb = null;
}

Expand Down
0