Skip to content
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.

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

Fix Symfony 5.2 compatibility because Kernel::preBoot() broke the current implementation #37

Merged
merged 16 commits into from
Apr 2, 2021

Conversation

t-richard
Copy link
Member

No description provided.

@t-richard t-richard changed the title Preboot symfony 52 See if Symfony 5.2 Kernel::preBoot() broke the bridge Mar 31, 2021
@t-richard
Copy link
Member Author

Did a really dumb test on this issue and it turns out the cache is not copied due the preBoot function being added in Symfony 5.2.

To test it, I required bref/symfony-bridge, extended the BrefKernel then added this in my own Kernel to add var_dump statements.

    public function handle(Request $request, int $type = HttpKernelInterface::MASTER_REQUEST, bool $catch = true)
    {
        var_dump('handle');
        var_dump(scandir('/tmp/cache/prod'));
        var_dump(scandir($_SERVER['LAMBDA_TASK_ROOT'].'/var/cache/prod'));
        return parent::handle($request, $type, $catch);
    }

    public function boot()
    {
        var_dump('boot');
        var_dump(scandir('/tmp/cache/prod'));
        var_dump(scandir($_SERVER['LAMBDA_TASK_ROOT'].'/var/cache/prod'));
        parent::boot();
    }

    protected function dumpContainer(ConfigCache $cache, ContainerBuilder $container, string $class, string $baseClass)
    {
        var_dump('dumpContainer');
        parent::dumpContainer($cache, $container, $class, $baseClass);
    }

You can see the dump below but it basically goes like so

  • handle
    • empty /tmp/var/cache
    • deployed cache with container abc
  • dump container
  • boot
    • container xyz in /tmp/var/cache
    • deployed cache with container abc

So Symfony is generating a new cache in /tmp and the deployed pre-warmed cache is never copied.

string(6) "handle"
bool(false)
array(12) {
  [0]=>
  string(1) "."
  [1]=>
  string(2) ".."
  [2]=>
  string(27) "App_KernelProdContainer.php"
  [3]=>
  string(32) "App_KernelProdContainer.php.lock"
  [4]=>
  string(32) "App_KernelProdContainer.php.meta"
  [5]=>
  string(35) "App_KernelProdContainer.preload.php"
  [6]=>
  string(16) "ContainerCGtKGrl"
  [7]=>
  string(15) "annotations.map"
  [8]=>
  string(25) "url_generating_routes.php"
  [9]=>
  string(30) "url_generating_routes.php.meta"
  [10]=>
  string(23) "url_matching_routes.php"
  [11]=>
  string(28) "url_matching_routes.php.meta"
}
string(13) "dumpContainer"
string(4) "boot"
array(8) {
  [0]=>
  string(1) "."
  [1]=>
  string(2) ".."
  [2]=>
  string(27) "App_KernelProdContainer.php"
  [3]=>
  string(32) "App_KernelProdContainer.php.lock"
  [4]=>
  string(32) "App_KernelProdContainer.php.meta"
  [5]=>
  string(35) "App_KernelProdContainer.preload.php"
  [6]=>
  string(16) "ContainerNNgOvfp"
  [7]=>
  string(15) "annotations.map"
}
array(12) {
  [0]=>
  string(1) "."
  [1]=>
  string(2) ".."
  [2]=>
  string(27) "App_KernelProdContainer.php"
  [3]=>
  string(32) "App_KernelProdContainer.php.lock"
  [4]=>
  string(32) "App_KernelProdContainer.php.meta"
  [5]=>
  string(35) "App_KernelProdContainer.preload.php"
  [6]=>
  string(16) "ContainerCGtKGrl"
  [7]=>
  string(15) "annotations.map"
  [8]=>
  string(25) "url_generating_routes.php"
  [9]=>
  string(30) "url_generating_routes.php.meta"
  [10]=>
  string(23) "url_matching_routes.php"
  [11]=>
  string(28) "url_matching_routes.php.meta"
}

@t-richard
Copy link
Member Author

Following investigation on why the tests are NOT failing here.

We are actually testing with the console which also uses the Kernel but the console is not calling preBoot (this function was introduced for some http related feature) so it doesn't fail and work as expected but in a console context.

We should probably change the tests to simulate an HttpRequest instead

@t-richard
Copy link
Member Author

Ok, I fixed the tests to fail on Symfony 5.2 where the behaviour of Kernel::handle() changed. I've also added PHP 8.0 to the version matrix because support was added in #34 but the CI, tests and dev-dependencies weren't updated.

This commit is a point at which the tests are failing but the bug was not resolved and we can see all 5.2 tests on all PHP versions are failing while others are fine.

image

I'll now work on making it all green again 🚥

@t-richard t-richard changed the title See if Symfony 5.2 Kernel::preBoot() broke the bridge Fix Symfony 5.2 compatibility because Kernel::preBoot() broke the current implementation Apr 2, 2021
Copy link
Member

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

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

Thank you for going through all of that!

@mnapoli mnapoli merged commit f5f7472 into brefphp:master Apr 2, 2021
@t-richard
Copy link
Member Author

@mnapoli you're welcome 🙂

That was easier than I thought once I understood why the tests were not failing on 5.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants