CacheWarmerAggregate handle deprecations logs #27421

ScullWM · 2018-05-30T10:36:59Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#27387
License	MIT

Actually the Web Debug Toolbar warning you about the deprecation messages thrown during the container built (#21502).
Cache warmup can throw deprecated message without any persistance, it may cause issue like #27387
This PR reproduce the same job for the cache warmer, and so on, handle deprecated messages during the warmup of Twig, Translator, Validator, Security and all kernel.cache_warmer services.

Here are the point that may be improvable in this PR:

Actually I've "duplicate" the callable used in the set_error_handler of the Kernel.
IMHO I think that Kernel and CacheWarmerAggregate have differents jobs and a trait may be a good solution to share this error handler setter without duplicating the code, but I'm a little bit lost about the repercussion of adding a Trait in the Kernel.
I've think about extending the CacheWarmerAggregate into a DeprecatedLogHandlingCacheWarmerAggregate to add the debug and containerClass argument, and declare it as the cache_warmer service only in debug mode (by declaring it in the DebugBundle/Resources/config/services.xml).

ScullWM · 2018-05-30T10:38:10Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml

@@ -32,6 +32,8 @@

        <service id="cache_warmer" class="Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerAggregate" public="true">
            <argument type="tagged" tag="kernel.cache_warmer" />
+            <argument>%kernel.debug%</argument>


CacheWarmerAggregate is annoted as final, so it shouldn't be BCBreak

nicolas-grekas

thanks for working on this, it's been missing a lot!

nicolas-grekas · 2018-05-31T10:47:30Z

src/Symfony/Component/HttpKernel/CacheWarmer/CacheWarmerAggregate.php

    {
        $this->warmers = $warmers;
+        $this->debug = $debug;
+        $this->containerPathPrefix = $containerPathPrefix;


I'd suggest to pass the path of the deprecation file instead, not only the prefix

$containerPathPrefix became $deprecationLogsFilepath with the right argument.

nicolas-grekas · 2018-05-31T10:48:25Z

src/Symfony/Component/HttpKernel/CacheWarmer/CacheWarmerAggregate.php


-            $warmer->warmUp($cacheDir);
+                file_put_contents($this->containerPathPrefix.'Deprecations.log', serialize(array_values($collectedLogs)));
+                restore_error_handler();


this call should be the 1st in the "if" so that we're sure it's always restored correctly

nicolas-grekas

LGTM, with one minor comment.

nicolas-grekas · 2018-05-31T12:57:08Z

src/Symfony/Component/HttpKernel/CacheWarmer/CacheWarmerAggregate.php


-            $warmer->warmUp($cacheDir);
+                if (file_exists($file = $this->deprecationLogsFilepath)) {


no need for the $file tmp var now

ScullWM · 2018-05-31T21:31:18Z

src/Symfony/Component/HttpKernel/CacheWarmer/CacheWarmerAggregate.php

            }
+        } finally {
+            if ($this->debug && true !== $previousHandler) {


I've also prepare a test scenario with a cacheWarmer who throw deprecations but here I'm directly in conflict with #25413 (fixing #24767). It can't pass in this condition during the test and it's right.

Here was my test code (CacheWarmerAggregateTest):

public function testWarmupWithDeprecationLogs() { $cacheDir = self::$cacheDir; $warmer = $this->getCacheWarmerMock(); $warmer ->expects($this->once()) ->method('warmUp') ->with($cacheDir) ->will($this->returnValue($this->throwDeprecation())); ; $deprecationLogsFilepath = self::$cacheDir.'/Deprecations.log'; $aggregate = new CacheWarmerAggregate(array($warmer), true, $deprecationLogsFilepath); $aggregate->warmUp($cacheDir); $this->assertTrue(file_exists($deprecationLogsFilepath)); $this->assertCount(1, unserialize(file_get_contents($deprecationLogsFilepath))); } protected function throwDeprecation() { @trigger_error('Deprecation Message', E_USER_DEPRECATED); }

Unsetting the env var temporarily should be doable?

I've try many solution, and even temporarily removing the defined constant (runkit_constant_remove), deprecations have a different behavior during phpunit test. Still looking to find a solution and test it.

ok, let's forget about it :):

ScullWM · 2018-06-01T11:43:03Z

Show rendering on symfony-demo:

xabbuh · 2018-06-02T18:32:02Z

src/Symfony/Component/HttpKernel/CacheWarmer/CacheWarmerAggregate.php

-                continue;
+        if ($this->debug) {
+            $collectedLogs = array();
+            $previousHandler = defined('PHPUNIT_COMPOSER_INSTALL');


Why do we need this?

If you're talking about the definition of PHPUNIT_COMPOSER_INSTALL it's related to #25413

nicolas-grekas · 2018-06-19T13:16:03Z

Thank you @ScullWM.

This PR was merged into the 4.2-dev branch. Discussion ---------- CacheWarmerAggregate handle deprecations logs | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes  | Fixed tickets | #27387 | License | MIT Actually the Web Debug Toolbar warning you about the deprecation messages thrown during the container built (#21502). Cache warmup can throw deprecated message without any persistance, it may cause issue like #27387 This PR reproduce the same job for the cache warmer, and so on, handle deprecated messages during the warmup of Twig, Translator, Validator, Security and all `kernel.cache_warmer` services. Here are the point that may be improvable in this PR: 1. Actually I've "duplicate" the callable used in the `set_error_handler` of the Kernel. IMHO I think that Kernel and CacheWarmerAggregate have differents jobs and a trait may be a good solution to share this error handler setter without duplicating the code, but I'm a little bit lost about the repercussion of adding a Trait in the Kernel. 2. I've think about extending the `CacheWarmerAggregate` into a `DeprecatedLogHandlingCacheWarmerAggregate` to add the debug and containerClass argument, and declare it as the `cache_warmer` service only in debug mode (by declaring it in the DebugBundle/Resources/config/services.xml). Commits ------- f03b8bb CacheWarmerAggregate handle deprecations logs

stof · 2018-06-19T13:34:25Z

What about having separate files for the warmer deprecations and the container deprecations ? It would avoid having to read the existing file to merge deprecations with the existing ones (which might cause issues if you run cache:warmup as the cache warmer could run multiple times for a single run of the container building, duplicating warmer deprecations)

carsonbot added Status: Needs Review Feature labels May 30, 2018

ScullWM commented May 30, 2018

View reviewed changes

ScullWM force-pushed the cachewarmer_deprecated_handler branch 2 times, most recently from 61eb2e1 to ed92377 Compare May 30, 2018 19:31

nicolas-grekas added this to the next milestone May 31, 2018

nicolas-grekas reviewed May 31, 2018

View reviewed changes

ScullWM force-pushed the cachewarmer_deprecated_handler branch from ed92377 to 1da0b71 Compare May 31, 2018 11:35

carsonbot removed the Status: Needs Review label May 31, 2018

nicolas-grekas approved these changes May 31, 2018

View reviewed changes

carsonbot added the Status: Reviewed label May 31, 2018

ScullWM force-pushed the cachewarmer_deprecated_handler branch from 1da0b71 to 3e473ee Compare May 31, 2018 21:15

ScullWM commented May 31, 2018

View reviewed changes

ScullWM force-pushed the cachewarmer_deprecated_handler branch 2 times, most recently from 4d499a7 to 1cbe807 Compare May 31, 2018 22:14

ScullWM force-pushed the cachewarmer_deprecated_handler branch from 1cbe807 to f6c3851 Compare June 1, 2018 14:32

xabbuh reviewed Jun 2, 2018

View reviewed changes

20uf approved these changes Jun 5, 2018

View reviewed changes

xabbuh approved these changes Jun 5, 2018

View reviewed changes

xabbuh added the FrameworkBundle label Jun 5, 2018

CacheWarmerAggregate handle deprecations logs

f03b8bb

ScullWM force-pushed the cachewarmer_deprecated_handler branch from f6c3851 to f03b8bb Compare June 5, 2018 22:13

nicolas-grekas merged commit f03b8bb into symfony:master Jun 19, 2018

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

fabpot mentioned this pull request Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

fabpot mentioned this pull request Nov 3, 2018

Release v4.2.0-BETA1 #29073

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CacheWarmerAggregate handle deprecations logs #27421

CacheWarmerAggregate handle deprecations logs #27421

ScullWM commented May 30, 2018 •

edited

Loading

ScullWM May 30, 2018

nicolas-grekas left a comment

nicolas-grekas May 31, 2018

ScullWM May 31, 2018

nicolas-grekas May 31, 2018

nicolas-grekas left a comment

nicolas-grekas May 31, 2018

ScullWM May 31, 2018

nicolas-grekas May 31, 2018 •

edited

Loading

ScullWM Jun 1, 2018

nicolas-grekas Jun 1, 2018

ScullWM commented Jun 1, 2018 •

edited

Loading

xabbuh Jun 2, 2018

ScullWM Jun 3, 2018

nicolas-grekas commented Jun 19, 2018

stof commented Jun 19, 2018


		$warmer->warmUp($cacheDir);
		if (file_exists($file = $this->deprecationLogsFilepath)) {

CacheWarmerAggregate handle deprecations logs #27421

CacheWarmerAggregate handle deprecations logs #27421

Conversation

ScullWM commented May 30, 2018 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas left a 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScullWM commented Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jun 19, 2018

stof commented Jun 19, 2018

ScullWM commented May 30, 2018 •

edited

Loading

nicolas-grekas May 31, 2018 •

edited

Loading

ScullWM commented Jun 1, 2018 •

edited

Loading