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

[FrameworkBundle] Simplified the MicroKernelTrait setup #19977

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

javiereguiluz
Copy link
Member

Fixes #19975.

@yceruto
Copy link
Member

yceruto commented Jun 19, 2024

Javier thanks for opening this PR so quickly :)

I was wondering if we could simplify the example even more by referencing the runtime autoload file instead and replace this code:

$kernel = new Kernel('dev', true);
$request = Request::createFromGlobals();
$response = $kernel->handle($request);
$response->send();
$kernel->terminate($request, $response);

with this other:

return static function (array $context) {
    return new Kernel($context['APP_ENV'], (bool) $context['APP_DEBUG']);
}

wdyt?

@javiereguiluz
Copy link
Member Author

I like your proposal Yonel. I updated the PR with your changes. Thanks for the review.

@@ -34,19 +34,12 @@ Next, create an ``index.php`` file that defines the kernel class and runs it:
use Symfony\Component\HttpKernel\Kernel as BaseKernel;
use Symfony\Component\Routing\Attribute\Route;

require __DIR__.'/vendor/autoload.php';
require_once dirname(__DIR__).'/vendor/autoload_runtime.php';
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to update the composer require section with symfony/runtime libs ;)

Copy link
Member

@yceruto yceruto Jun 20, 2024

Choose a reason for hiding this comment

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

In that section only symfony/framework-bundle is required actually, the other ones are transitive dependencies anyway, not sure we wanted to make them explicit though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! I think that, generally speaking, we don't list transitive dependencies in the docs, so let's remove anything that is not strictly needed. Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants