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

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

Status: needs review

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

Not using proxies is better for sure :)

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

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.

And I just tested the PR,

1. it keeps writing to the session, event if nothing changes

class HomepageControllerextends AbstractController{    #[Route('/')]publicfunctionindex(        #[MapSessionParameter()]Foobar$foobar4,    ):Response    {dump($foobar4);return$this->render('homepage/index.html.twig', [        ]);    }}class Foobar{publicfunction__construct(    ) {    }}

1. I brokenALL FPM worker many time by playing with types. I ransymfony:server and had to restart it each time

Is it a commun thing to store object in session? It feel a bit wrong to me.

The goal of this PR is not to allow storing object but reducing type error and wrong session key error (username vs userName).
If you expect to have an int in session, the property of the object or the method on the interface will require you to provide the right type.
And while the typo part is a little less present in the current version, as it depend mainly on a variable name, it may be easier to spot.

People need to deal with BC between 2 deploys...

Didn't really think about this point. I guess you can use the fact that there is support for interface to leverage this point.
In my mind you have only one or two property per object, so there should not be too much issue with BC, at least not new issues compared to calling session->get directly

My point about objects it that is encourage people to store objects in the session, and IMHO this is a terrible idea. So, even if this PR looks nice at glance, it's not a good idea.

But we do ourselves put value objects in the session, I see no issue, with some care of course.

I've added a new commit to add support for the union/intersection type, they are considered like the interface (need to provide a default value or make them nullable).
This also fix if you usedFooInterface|null $var instead ofFooInterface|null $var = null

There is one point that i didn't find an easy/effective way to do : Check if the data in the session match the expected type to trigger an exception saying that the type in session was not compliant. Instead, the user will have a message like
Argument #N ($browsingSessionContext) must be of type (Foo&Bar)|null, Baz given

Is there a method like that ? (instanceof and is_a doesn't seem to handle union types)
Or can we leave the php default error ?

Two very genuine questions about this attribute, sorry if it has been mentionned in one of the previous threads.

First: Isn't this the only valueresolver thatwrite things? If so I guess this is something that should be written in bold in the documentation.

Second (related) question: does this mean if a controller is protected by aIsGranted attribute --as the request arguments have to be resolved before calling the authorisation request listener-- with this PR the resolver will save data in sessionbefore authorisation. In case of access denied, or any error in fact... the value is not rollback to its previous state, or did I miss something ?

First: Isn't this the only value resolver that write things? If so I guess this is something that should be written in bold in the documentation.

This is not desired to me, I would remove theSession::set() call.

Two very genuine questions about this attribute, sorry if it has been mentionned in one of the previous threads.

This was not mentionned before, and for the second question, i have not a clue, so i'll dive more into this part, so thank you :)

First: Isn't this the only valueresolver thatwrite things? If so I guess this is something that should be written in bold in the documentation.

It may be the only resolver that write, it was to avoid the case where depending of if the session was already set or not, the value would save automatically or not.
If the behavior stay like this, the documentation will be updated to indicate this part.

Second (related) question: does this mean if a controller is protected by aIsGranted attribute --as the request arguments have to be resolved before calling the authorisation request listener-- with this PR the resolver will save data in sessionbefore authorisation. In case of access denied, or any error in fact... the value is not rollback to its previous state, or did I miss something ?

I don't know enough about the exact order for this process, i'll deep dive into this part to understand more.
In this case, either you already have a value, then the value is only read from session, or you don't, then a default value is written in session. It can break something if you depend on thesession->has.

From all the feedback, i get that the fact that it is an object that is stored and we write automatically in the session is not the intended behavior.
We can either rollback to the original version where the class was proxied to call session->get/set instead of reading properties.
Or make the autowire attribute work only with argument that have default value or nullable with no auto-set, so the user should always be saving manually and handle creation (maybe allow to autowire the session or the setter).