[Yaml] don't override internal PHP constants #12372

xabbuh · 2014-11-01T12:56:51Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#11783
License	MIT
Doc PR

fabpot · 2014-11-01T14:22:29Z

Would it make sense to make similar changes elsewhere in the code where we also override PHP constants? At least where speed is not critical.

xabbuh · 2014-11-01T15:37:13Z

Which parts do you have in mind?

fabpot · 2014-11-02T01:12:44Z

I think almost all of them:

src/Symfony/Bridge/Twig/Extension/CodeExtension.php
src/Symfony/Bundle/FrameworkBundle/Templating/Helper/CodeHelper.php
src/Symfony/Component/ClassLoader/ClassMapGenerator.php
src/Symfony/Component/Console/Input/InputDefinition.php
src/Symfony/Component/Debug/ExceptionHandler.php
src/Symfony/Component/HttpKernel/Fragment/HIncludeFragmentRenderer.php
src/Symfony/Component/Intl/Resources/bin/common.php
src/Symfony/Component/Process/Tests/PipeStdinInStdoutStdErrStreamSelect.php
src/Symfony/Component/Templating/PhpEngine.php
src/Symfony/Component/Yaml/Exception/ParseException.php

xabbuh · 2014-11-16T13:32:56Z

I updated removed other redefinitions of internal PHP constants. However, src/Symfony/Component/Intl/Resources/bin/common.php and src/Symfony/Component/Process/Tests/PipeStdinInStdoutStdErrStreamSelect.php define some constants, but those are not related to the PHP core itself. So I didn't modify them.

fabpot · 2014-11-16T17:52:38Z

src/Symfony/Component/Templating/PhpEngine.php

@@ -477,9 +473,14 @@ protected function initializeEscapers()
                 * @return string the escaped value
                 */
                function ($value) use ($that) {
+                    if (version_compare(PHP_VERSION, '5.4.0', '>=')) {


This should be done outside of the function to avoid computing it each time a value is escaped.

fabpot · 2014-11-16T17:53:18Z

👍 (after the escape() method is optimized)

fabpot · 2014-11-16T18:04:53Z

Thank you @xabbuh.

This PR was merged into the 2.3 branch. Discussion ---------- [Yaml] don't override internal PHP constants | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #11783 | License | MIT | Doc PR | Commits ------- 376cc03 don't override internal PHP constants

stof · 2014-11-17T13:53:26Z

src/Symfony/Bridge/Twig/Extension/CodeExtension.php

@@ -176,7 +172,13 @@ public function formatFile($file, $line, $text = null)
        $text = "$text at line $line";

        if (false !== $link = $this->getFileLink($file, $line)) {
-            return sprintf('<a href="%s" title="Click to open this file" class="file_link">%s</a>', htmlspecialchars($link, ENT_QUOTES | ENT_SUBSTITUTE, $this->charset), $text);
+            if (version_compare(PHP_VERSION, '5.4.0', '>=')) {


Would it be worth using PHP_VERSION_ID > 50400 instead ? This way, OPCache could optimize it (it will inline the constant value for sure, and it could be smart enough to inline the comparison result as it would compare 2 constants which would then eliminate the useless branch entirely)

that does make sense. 👍

That's indeed a good idea (see #12497).

arnaugm · 2014-11-29T09:46:43Z

I've just cloned the symfony repository in the hackday of the SymfonyCon and I'm running the tests. I'm getting this strange error:

1) Symfony\Component\Form\Tests\Extension\Csrf\CsrfProvider\DefaultCsrfProviderTest::testGenerateCsrfToken
PHPUnit_Framework_Exception: PHP Notice:  Constant SYMFONY_TRAIT already defined in /home/arnau/code/symfony/src/Symfony/Component/ClassLoader/ClassMapGenerator.php on line 15
PHP Stack trace:
PHP   1. {main}() -:0
PHP   2. require_once() -:2143
PHP   3. define() /home/arnau/code/symfony/src/Symfony/Component/ClassLoader/ClassMapGenerator.php:15

Notice: Constant SYMFONY_TRAIT already defined in /home/arnau/code/symfony/src/Symfony/Component/ClassLoader/ClassMapGenerator.php on line 15

Call Stack:
    0.0037     825536   1. {main}() -:0
    0.5145  171491904   2. require_once('/home/arnau/code/symfony/src/Symfony/Component/ClassLoader/ClassMapGenerator.php') -:2143
    0.5145  171492048   3. define() /home/arnau/code/symfony/src/Symfony/Component/ClassLoader/ClassMapGenerator.php:15

After having a look at it with @weaverryan we arrived to that issue which is apparently related to the error.

This seems to be very related to my specific configuration, I'm using:
OS: Ubuntu 14.04.1 LTS
PHP: 5.5.9-1ubuntu4.5
PHPUnit: 3.7.20

xabbuh · 2014-11-29T09:56:53Z

@arnaugm I'll have a look at it. How do you execute the tests?

arnaugm · 2014-11-29T10:15:17Z

Just with

phpunit

Thanks!

desarrolla2 · 2014-11-29T10:26:49Z

I have the same error. Same version of OS and PHP.
PHPUnit 4.3.1

Y tried this

phpunit --strict

and failing too but this is working

phpunit --filter="DefaultCsrfProviderTest::testGenerateCsrfToken"

And this working too

phpunit --filter="DefaultCsrfProviderTest"

I have the error in a lot of test.

xabbuh · 2014-11-29T11:37:05Z

@arnaugm @desarrolla2 Can you both please check if the error disappears when you modify the top of the src/Symfony/Component/ClassLoader/ClassMapGenerator.php file like this:

if (!defined('SYMFONY_TRAIT')) {
    if (PHP_VERSION_ID >= 50400) {
        define('SYMFONY_TRAIT', T_TRAIT);
    } else {
        define('SYMFONY_TRAIT', 0);
    }
}

arnaugm · 2014-11-29T12:33:36Z

Yes, that makes the test pass.

xabbuh · 2014-11-29T12:49:35Z

Thanks for the confirmation @arnaugm. I opened a pull request (see #12716).

…dosten) This PR was squashed before being merged into the 2.6 branch (closes #14515). Discussion ---------- Do not override PHP constants, only use when available | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT In #12372, the override of internal constants has been deleted, but the definition of `JSON_PRETTY_PRINT` if not available is missing in that PR. Commits ------- d5cc056 Do not override PHP constants, only use when available

xabbuh changed the title ~~[Yaml] don't override built-in PHP constants~~ [Yaml] don't override internal PHP constants Nov 1, 2014

xabbuh force-pushed the issue-11783 branch from 336422c to e1f8b1b Compare November 1, 2014 12:57

unkind mentioned this pull request Nov 4, 2014

minimal PHP version requirement for Symfony 3.0 #12402

Closed

xabbuh force-pushed the issue-11783 branch from e1f8b1b to 9d6e0ed Compare November 16, 2014 13:27

fabpot reviewed Nov 16, 2014
View reviewed changes

don't override internal PHP constants

376cc03

xabbuh force-pushed the issue-11783 branch from 9d6e0ed to 376cc03 Compare November 16, 2014 17:59

fabpot merged commit 376cc03 into symfony:2.3 Nov 16, 2014

xabbuh deleted the issue-11783 branch November 16, 2014 18:07

stof reviewed Nov 17, 2014
View reviewed changes

xabbuh mentioned this pull request Nov 17, 2014

compare version using PHP_VERSION_ID #12497

Merged

xabbuh mentioned this pull request Nov 29, 2014

[ClassLoader] define constant only if it wasn't defined before #12716

Merged

xabbuh mentioned this pull request Nov 29, 2014

[Hackday] - trigger errors for deprecated classes #12694

Closed

dosten mentioned this pull request May 1, 2015

Do not override PHP constants, only use when available #14515

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Yaml] don't override internal PHP constants #12372

[Yaml] don't override internal PHP constants #12372

xabbuh commented Nov 1, 2014

fabpot commented Nov 1, 2014

xabbuh commented Nov 1, 2014

fabpot commented Nov 2, 2014

xabbuh commented Nov 16, 2014

fabpot Nov 16, 2014

xabbuh Nov 16, 2014

fabpot commented Nov 16, 2014

fabpot commented Nov 16, 2014

stof Nov 17, 2014

fabpot Nov 17, 2014

xabbuh Nov 17, 2014

arnaugm commented Nov 29, 2014

xabbuh commented Nov 29, 2014

arnaugm commented Nov 29, 2014

desarrolla2 commented Nov 29, 2014

xabbuh commented Nov 29, 2014

arnaugm commented Nov 29, 2014

xabbuh commented Nov 29, 2014

[Yaml] don't override internal PHP constants #12372

[Yaml] don't override internal PHP constants #12372

Conversation

xabbuh commented Nov 1, 2014

fabpot commented Nov 1, 2014

xabbuh commented Nov 1, 2014

fabpot commented Nov 2, 2014

xabbuh commented Nov 16, 2014

fabpot Nov 16, 2014

Choose a reason for hiding this comment

xabbuh Nov 16, 2014

Choose a reason for hiding this comment

fabpot commented Nov 16, 2014

fabpot commented Nov 16, 2014

stof Nov 17, 2014

Choose a reason for hiding this comment

fabpot Nov 17, 2014

Choose a reason for hiding this comment

xabbuh Nov 17, 2014

Choose a reason for hiding this comment

arnaugm commented Nov 29, 2014

xabbuh commented Nov 29, 2014

arnaugm commented Nov 29, 2014

desarrolla2 commented Nov 29, 2014

xabbuh commented Nov 29, 2014

arnaugm commented Nov 29, 2014

xabbuh commented Nov 29, 2014