[HttpKernel] Add MapSessionParameter to map session parameters to controllers

[jtattevin]

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
License	MIT
Doc PR	symfony/symfony-docs#19728

This attribute allow to map an object from/to the session. The read/write to the public properties are proxied to the session using the property name as a key, or the value defined by the attribute SessionKey. There is no prefix, so if two class use the same property, they will read/write to the same session entry, which is intended to have variable shared between context if needed.

This allow to type property and have ide autocompletion, static validation and runtime validation of type. The default value from the property is also used as a default value when reading from session.

Using this method reduce the errors related to a wrong key used or inconsistent default value. It also help seing which key are currently used and avoid conflicts

Calling isset will return if the property is defined in session but will not check if the value is null. Calling unset remove the entry from the session

For example, we can create a class like this :

class BrowsingSessionContext
{
    public string $locale = "en";

    public bool $preferDarkTheme = false;
}

and use it in a controller :

#[Route("/", name: "home")]
class DefaultController extends AbstractController
{
    public function __invoke(
        Request $request,
        #[MapSessionContext] BrowsingSessionContext $browsingSessionContext,
    ) {
        $localeSelector                 = $this
            ->createFormBuilder($browsingSessionContext)
            ->add("locale", TextType::class)
            ->add("preferDarkTheme", CheckboxType::class)
            ->add("submit", SubmitType::class)
            ->getForm()
        ;
        $localeSelector->handleRequest($request);
        if ($localeSelector->isSubmitted() && $localeSelector->isValid()) {
            return $this->redirectToRoute("home");
        }

        return $this->render("index.html.twig", [
            "value"          => $browsingSessionContext->locale,
            "localeSelector" => $localeSelector,
        ]);
    }
}

The term session context come from the idea that we get a lot of information that are often together. I avoided to use something too close of Object to avoid confusion with the SessionInterface object.

This pull request is based in HttpKernel to be close to SessionValueResolver but i'm not confident if the namespaces used are the right ones.

I'm not fond of the eval to create the anonymous class, but didn't managed to create an anonymous class without that. Also, i'm not sure if it's possible to create a proxy during container build in this context.

[carsonbot]

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 7.1 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

[carsonbot]

Hey!

Thanks for your PR. You are targeting branch "7.1" but it seems your PR description refers to branch "7.2".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

[jtattevin]

Status: needs review

[nicolas-grekas]

Thanks for submitting this. But do we need these magic proxies at all? If we store an object directly in the session, then mutating this object updates the session. This means we don't need any magic to me, keeping a reference to an object stored in the session is enough. Did you consider this?

Here is a small controller I wrote to verify my claim:

        if (!$request->getSession()->has('foo')) {
            $foo = new \stdClass();
            $foo->bar = 0;
            $request->getSession()->set('foo', $foo);
        } else {
            $foo = $request->getSession()->get('foo');
        }

        dump(++$foo->bar);

[jtattevin]

Thank for the feedback.

I didn't think about injecting the object itself, i think it would work yes and be simpler.

Initially i made it this way to migrate a project that was using directly the set/get without constant, so the goal was mainly to proxy the calls and avoid issues like reading 'locale' but writing to '_locale'. In session i wanted to have the individual keys.

I'm not sure which method would be better

[nicolas-grekas]

Not using proxies is better for sure :)

[jtattevin]

Update done, i've removed the magic trait and also the possibility to specify the session key for each property.
The class FQDN is the entry in the session

[nicolas-grekas]

I like this idea :)

[jtattevin]

Update done, good idea to allow the interface and thank for all the feedback.

I made a change on which i'm not 100% certain if it was the right call :
If you type an interface, you need to make the parameter nullable or provide a default value.

While it is not always mandatory because if there is already a value in session, it would work without a default value, it may lead to an error later if the controller is called without setting the session before. (For example, the server restart and clear the session).

This is a case that may be complicated to detect during development, so trigger the error even if it was possible to continue without error.

[nicolas-grekas]

This looks good to me, I only have CS-related comments.

[nicolas-grekas]

Nice :)