[HttpKernel] fix ErrorException in CacheWarmerAggregate #43501

Ahummeling · 2021-10-14T11:58:15Z

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets
License	MIT
Doc PR

I ran into an ErrorException reloading after some changes. I suppose this triggered a cache warmup call. This didn't go as expected, because $previousLogs = unserialize(file_get_contents($this->deprecationLogsFilepath)); set $previousLogs to false instead of the expected array. I think it makes sense to only call array_merge if $previousLogs was successfully instantiated as an array.

My IDE also pointed out that $collectedLogs was possible undefined, so moving its declaration outside of the if block resolved that.

Stacktrace:

ErrorException:
Warning: array_merge(): Expected parameter 1 to be an array, bool given

  at /srv/vendor/symfony/http-kernel/CacheWarmer/CacheWarmerAggregate.php:106
  at Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerAggregate->warmUp('/srv/var/cache/local')
     (/srv/vendor/symfony/http-kernel/Kernel.php:584)
  at Symfony\Component\HttpKernel\Kernel->initializeContainer()
     (/srv/vendor/symfony/http-kernel/Kernel.php:786)
  at Symfony\Component\HttpKernel\Kernel->preBoot()
     (/srv/vendor/symfony/http-kernel/Kernel.php:187)
  at Symfony\Component\HttpKernel\Kernel->handle(object(Request))
     (/srv/public/index.php:44)

carsonbot · 2021-10-14T11:58:19Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 5.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

nicolas-grekas · 2021-10-28T15:04:50Z

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

        if ($collectDeprecations = $this->debug && !\defined('PHPUNIT_COMPOSER_INSTALL')) {
-            $collectedLogs = [];


this should be reverted, the IDE is just missing a brain :)

fair enough, will take care of that.
Reverting that is:p

derrabus

Looks good to me. Is it possible to write a test for that change?

Ahummeling · 2021-10-28T16:11:25Z

Looks good to me. Is it possible to write a test for that change?

Oh I hadn't looked into that actually.
I'll see if there are related tests and see if it's possible to test it. And if so, write a test:)

Ahummeling · 2021-10-29T09:45:03Z

So @derrabus I am running into an issue here attempting to test this. The relevant codeblock is only executed when the constant PHPUNIT_COMPOSER_INSTALL is not defined. I found that I can "undefine" a constant using uopz_undefine, however, I'd have to install the extension in the ci flow and I am not really sure it's worth the hassle.
Maybe I am overthinking this, I am open to suggestions, but as it stands right now, I'm unable to test it.

nicolas-grekas · 2021-10-29T09:46:45Z

I'm not sure this can be tested, because I suspect this might happen only in some race conditions.

Ahummeling · 2021-10-29T09:49:09Z

I'm not sure this can be tested, because I suspect this might happen only in some race conditions.

I am quite sure you can replicate it without race conditions by truncating the file to size 0 before warming cache
as unserialize(file_get_contents('empty_file')) returns false. Which causes a type error as it's not an array

nicolas-grekas · 2021-10-29T09:52:02Z

Oh, can you figure out a test case then?

Ahummeling · 2021-10-29T09:54:30Z

Yeah give me a few minutes and I'll push my testcase

Ahummeling · 2021-10-29T10:03:34Z

So without the \is_array check in there, running with php8.0, I get the expected error:

There was 1 error:

1) Symfony\Component\HttpKernel\Tests\CacheWarmer\CacheWarmerAggregateTest::testWarmupEmptyFile
TypeError: array_merge(): Argument #1 must be of type array, bool given

With the \is_array check, it runs fine. I am a little stuck looking for a cleaner solution for getting the $collectDeprecations to be true.
Maybe the \defined('PHPUNIT_COMPOSER_INSTALL') could be taken out? I am really not sure about this part

nicolas-grekas · 2021-10-29T16:02:42Z

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

@@ -50,7 +52,7 @@ public function enableOnlyOptionalWarmers()
     */
    public function warmUp($cacheDir)
    {
-        if ($collectDeprecations = $this->debug && !\defined('PHPUNIT_COMPOSER_INSTALL')) {
+        if ($collectDeprecations = $this->debug && (!\defined('PHPUNIT_COMPOSER_INSTALL') || file_exists(CacheWarmerAggregateTest::$emptyFile))) {


please revert this.
I'm fine if we don't test, compared to adding it :)

Yes!
I commited it to show why I thought I couldn't test it, and hoped that maybe I overlooked something.
I will revert the last commit and it should be good to go

fabpot · 2021-10-30T08:58:15Z

Thank you @Ahummeling.

carsonbot added Status: Needs Review Bug labels Oct 14, 2021

carsonbot added this to the 4.4 milestone Oct 14, 2021

Ahummeling force-pushed the fix-http-kernel-cache-warmer-aggregate branch from 06edb66 to e623192 Compare October 14, 2021 12:00

nicolas-grekas changed the title ~~fix ErrorExcception in CacheWarmerAggregate~~ [HttpKernel ]fix ErrorException in CacheWarmerAggregate Oct 28, 2021

nicolas-grekas changed the title ~~[HttpKernel ]fix ErrorException in CacheWarmerAggregate~~ [HttpKernel] fix ErrorException in CacheWarmerAggregate Oct 28, 2021

nicolas-grekas approved these changes Oct 28, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 28, 2021

carsonbot changed the title ~~[HttpKernel] fix ErrorException in CacheWarmerAggregate~~ fix ErrorException in CacheWarmerAggregate Oct 28, 2021

nicolas-grekas added the HttpKernel label Oct 28, 2021

carsonbot changed the title ~~fix ErrorException in CacheWarmerAggregate~~ [HttpKernel] fix ErrorException in CacheWarmerAggregate Oct 28, 2021

derrabus approved these changes Oct 28, 2021

View reviewed changes

fix ErrorExcception in CacheWarmerAggregate

2eaa19f

Ahummeling force-pushed the fix-http-kernel-cache-warmer-aggregate branch from e623192 to 2eaa19f Compare October 29, 2021 09:47

nicolas-grekas reviewed Oct 29, 2021

View reviewed changes

Ahummeling force-pushed the fix-http-kernel-cache-warmer-aggregate branch from fadafdc to 2eaa19f Compare October 29, 2021 19:06

fabpot approved these changes Oct 30, 2021

View reviewed changes

fabpot merged commit 415592a into symfony:4.4 Oct 30, 2021

This was referenced Nov 22, 2021

Release v4.4.34 #44193

Merged

Release v5.3.11 #44194

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpKernel] fix ErrorException in CacheWarmerAggregate #43501

[HttpKernel] fix ErrorException in CacheWarmerAggregate #43501

Ahummeling commented Oct 14, 2021

carsonbot commented Oct 14, 2021

nicolas-grekas Oct 28, 2021

Ahummeling Oct 28, 2021

derrabus left a comment

Ahummeling commented Oct 28, 2021

Ahummeling commented Oct 29, 2021

nicolas-grekas commented Oct 29, 2021

Ahummeling commented Oct 29, 2021 •

edited

Loading

nicolas-grekas commented Oct 29, 2021

Ahummeling commented Oct 29, 2021

Ahummeling commented Oct 29, 2021

nicolas-grekas Oct 29, 2021

Ahummeling Oct 29, 2021

fabpot commented Oct 30, 2021

		if ($collectDeprecations = $this->debug && !\defined('PHPUNIT_COMPOSER_INSTALL')) {
		$collectedLogs = [];

[HttpKernel] fix ErrorException in CacheWarmerAggregate #43501

[HttpKernel] fix ErrorException in CacheWarmerAggregate #43501

Conversation

Ahummeling commented Oct 14, 2021

carsonbot commented Oct 14, 2021

nicolas-grekas Oct 28, 2021

Choose a reason for hiding this comment

Ahummeling Oct 28, 2021

Choose a reason for hiding this comment

derrabus left a comment

Choose a reason for hiding this comment

Ahummeling commented Oct 28, 2021

Ahummeling commented Oct 29, 2021

nicolas-grekas commented Oct 29, 2021

Ahummeling commented Oct 29, 2021 • edited Loading

nicolas-grekas commented Oct 29, 2021

Ahummeling commented Oct 29, 2021

Ahummeling commented Oct 29, 2021

nicolas-grekas Oct 29, 2021

Choose a reason for hiding this comment

Ahummeling Oct 29, 2021

Choose a reason for hiding this comment

fabpot commented Oct 30, 2021

Ahummeling commented Oct 29, 2021 •

edited

Loading