I ran into anErrorException 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 tofalse instead of the expected array. I think it makes sense to only callarray_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 added Status: Needs Review Bug labels

Oct 14, 2021

carsonbot added this to the4.4 milestone

Oct 14, 2021

Copy link

carsonbot commentedOct 14, 2021

Hey!

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

Symfony has acontribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (seehttps://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://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

Ahummeling force-pushed thefix-http-kernel-cache-warmer-aggregate branch from06edb66 toe623192Compare

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

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

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

Copy link

Member

nicolas-grekasOct 28, 2021

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

AhummelingOct 28, 2021

Choose a reason for hiding this comment

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

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

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

Copy link

Member

derrabus left a comment

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

Ahummeling commentedOct 28, 2021

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:)

Copy link

ContributorAuthor

Ahummeling commentedOct 29, 2021

So@derrabus I am running into an issue here attempting to test this. The relevant codeblock is only executed when the constantPHPUNIT_COMPOSER_INSTALL is not defined. I found that I can "undefine" a constant usinguopz_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.

Copy link

Member

nicolas-grekas commentedOct 29, 2021

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

fix ErrorExcception in CacheWarmerAggregate

2eaa19f

Ahummeling force-pushed thefix-http-kernel-cache-warmer-aggregate branch frome623192 to2eaa19fCompare

October 29, 2021 09:47

Copy link

ContributorAuthor

Ahummeling commentedOct 29, 2021•
edited
Loading

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
asunserialize(file_get_contents('empty_file')) returnsfalse. Which causes a type error as it's not an array

Copy link

Member

nicolas-grekas commentedOct 29, 2021

Oh, can you figure out a test case then?

Copy link

ContributorAuthor

Ahummeling commentedOct 29, 2021

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

Copy link

ContributorAuthor

Ahummeling commentedOct 29, 2021

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::testWarmupEmptyFileTypeError: 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 reviewed

Oct 29, 2021

View reviewed changes

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

		publicfunctionwarmUp($cacheDir)
		{
		if ($collectDeprecations =$this->debug && !\defined('PHPUNIT_COMPOSER_INSTALL')) {
		if ($collectDeprecations =$this->debug &&(!\defined('PHPUNIT_COMPOSER_INSTALL') \|\|file_exists(CacheWarmerAggregateTest::$emptyFile))) {

Copy link

Member

nicolas-grekasOct 29, 2021

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

AhummelingOct 29, 2021

Choose a reason for hiding this comment

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

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