Tags when not using SensioFrameworkExtraBundle

[tjveldhuizen]

Since SensioFrameworkExtraBundle, we have switched from the ParamConverters to argument resolvers. This means, that the converted parameters are not longer available in the TagListener, resulting in issues generating tags based on the object ID, while we use slugs in our URL's.
As far as I understand, the Param converters, as their name says, literally replace the parameter in the request with the object. However, argument resolvers create an array of arguments used to execute the controller. Unfortunately they are not posted to the ResponseEvent listeners. Should the TagListener be bound to the ControllerArgumentsEvent, in this case?

[dbu]

i am not sure if i fully understand. the TagListener gathers tags from the annotations and sets the tags on the response object. do we have logic in this bundle that interacts with param converters?

if you have custom logic about adding tags, you need to make sure to set those tags on the tag manager service before the TagListener of this bundle is triggered on the response.

[tjveldhuizen]

My tag is, for example:

#[Route(path: '/{team}', methods: ['GET'])]
#[Tag('team', expression: "'team-'~team.getId()")]
public function getAction(Team $team): Response
{
    // ...
}

When using param converters, the team in the expression is an instance of Team, but when using argument resolvers, it is the string from the URL. And <string>.getId() obviously won't work.

[dbu]

ah, now i get it, thank you!

if we would calculate the tags in the ControllerArgumentsEvent, the arguments would be of the correct type, right?

then we should indeed change that. from FOSHttpCacheBundle point of view, it comes to the same at which point we update the tags (barring somebody hacking around with the reqeust object while processing it, but that would be very weird application design).

when using the old ParamConverters, are the objects also available in ControllerArgumentsEvent? could we just switch to that event always, or do we have to worry about BC for sensio bundle (or old symfony versions)?

would you be motivated to do the refactoring?

[tjveldhuizen]

I'll give it a try. I'm the person hacking around with the request object now, as a temporary workaround. And I don't like to do that, so a fix is necessary :)

I'm afraid there is a BC-breaking situation, though: to be backwards compatible, the requests attributes have to remain available as parameters in the expression. At first, I wanted to insert array_merge($request->attributes->all(), $arguments) as parameters in the expression evaluator, but that may surprise users who expect them to be strings, since they will be object instances in the new situation (see example below, given Team has no __toString()).

I can think of the following options:

1. Don't add the individual request attributes to the expression at all (BC-break, fixable by using the already available request object in the expression)
2. Accept that controller arguments overwrite request attributes (BC-break, somewhat magic, fixable by using the already available request object in the expression)
3. New tag parameter for expressions with the controller arguments #[Tag(expression_with_arguments: ...)] (No BC-break, but ugly)
4. Extra tag parameter to choose which parameters should be used #[Tag(expression: ..., arguments: true}] (No BC-break)

I think the last option is the best for now, in the next major version it might be nice to switch to option 1. Or do yo know a different solution?

#[Route(path: '/{team}', methods: ['GET'])]
#[Tag('team', expression: "'team-'~team")]
public function getAction(Team $team): Response
{
    // ...
}

[dbu]

thanks for the analysis. i agree about the BC break risk, as newer users probably rely on that behaviour. we could do some prefix for the controller argument names but thats really tricky.

version 3/4 are problematic because they would not allow to access other attributes anymore. what if we provide the controller arguments as a named collection? some name like controller_arguments? when needing a controller argument, one would access that through that collection.

[tjveldhuizen]

You mean other attributes, different than controller arguments and the request. That's true, I didn't think of that.

Technically, such a named collection also is a BC-break, however it would be weird if anybody names his attribute 'controller_arguments'. I don't like the length of that, though, mainly since it will be the main way to use controller arguments when the SensioFrameworkExtraBundle is gone. I think most expression users build their tags from the controller arguments and it would be unfriendly if they have to use controller_arguments. all the time. Maybe arguments or even args are options, but that increases the risk of BC-problems.

What if #[Tag(expression: "'team-'~team.getId()", arguments: true)] 'just' overwrites attributes with the same name as controller arguments, like the request in the code below? Would that be to magic?

Of course, a different attribute and/or annotation can also be an option.

What would be the solution if starting from scratch, ignoring BC? Namespaces/named collections for attr., args., req.? Maybe it gives a good insight if we approach it from that direction.

FOSHttpCacheBundle/src/EventListener/TagListener.php

Lines 172 to 174 in 3243682

	$values = $request->attributes->all();
	// if there is an attribute called "request", it needs to be accessed through the request.
	$values['request'] = $request;

[dbu]

the named collection could indeed hide things, and the shorter its name the more likely.

that said, we can totally do a new major version to avoid BC breaks. thinking about it a bit more, directly accessing values seems neat but can always lead to hiding something. some people will do controller arguments called args|arguments|attr|attribute

i actually like the idea of always namespacing, it makes it so much more obvious what one is referring to, and it neatly solves all name collision risk.

if we want an upgrade path, we could start providing attributes and arguments (but just omit them in case of a name collision) and for the next major we stop pushing all the attributes directly into the context.

i feel that typing a bit more in attributes / arguments is worth it for better readability and being really self-explanatory. request is already provided with the full name. wdyt?

[tjveldhuizen]

Since the length of tags should not be too long, it will be exceptional for users to have many parameters in their expression, so length should not be an option. I think 'arguments', 'attributes' and 'request' (or maybe the singular versions?) are good.

I like the idea of an upgrade path adding them, but not replacing existing attributes. So if a user has the attribute 'arguments', they had to change that first, before being able to use a 'real' argument in their expression.

[dbu]

okay, lets go with the full words. symfony Request calls it attributes, so i would go with plural for attributes and arguments.

the attributes would also be available as request.attributes.XY but that is getting a bit long so i like providing them in an attributes collection.

[tjveldhuizen]

Just a short update: due to other activities, I am not able to create this PR at least the coming two weeks. If somebody else can, please do, otherwise I'll look into it later.

[dbu]

in #611 we removed the annotation support and solely use attributes.

i hope to release 3.x soon, so if you find time to look into it, now would be a great moment to do it.

[usu]

Is this a blocking point for release of 3.x? Or is this something that could be fixed/added later?

[dbu]

it is not a blocker. would be nice to find a good solution, but better live with limited functionality here than rush a bad solution now that needs more BC breaks down the road.