Store the referrer by default in backend scope

[fritzmg]

#7190

This implements my opt-out suggestion from #7190 (comment).

Any back end route will now store the referrer by default - but you can opt-out via _store_referrer: false in your route defaults/attributes.

/cc @ameotoko

[leofeyer]

What about the BackendCsvImportController?

[fritzmg]

What about the BackendCsvImportController?

BackendCsvImportController has no actual routes. It is only used for back end module callbacks, as far as I can see.

[leofeyer]

Thank you @fritzmg.

[fritzmg]

Make the configuration explicit

Imho it should be the other way around for safety. i.e. we want to make sure that only the contao_backend route from that controller gets the referrer stored - as this was the previous state. Besides, now the definition is inconsistent with the other back end controller route definitions. But it's a minor nitpick.

[leofeyer]

Besides, now the definition is inconsistent with the other back end controller route definitions.

What do you mean? The routes now explicitly have _store_referrer: false, just like the other back end controllers. It was inconsistent before, when the main controller had _store_referrer: true, even though false is the only handled option.

[fritzmg]

Before your change the general route definition on all the classes had "_store_referrer" = false. In your change you removed that from the general route definition of the BackendController class alone - but not from the other two back end controllers (hence the inconsistency). Again, the idea was to ensure in the general route definition that the referrer is not stored for all the routes in the BackendController - and only enable it specifically for the contao_backend route.

For the other back end controllers it is not so much of an issue. While they do have a general route definition on their respective classes, they only have one route anyway - which is defined on the __invoke method (imho this should be consolidated, but that's another issue).

[leofeyer]

#7232 fixes the inconsistency.

My reasoning is as follows: If an option can be anything or false, that is a classic opt-out case. And opt-out means that it is on by default and off in certain cases. Therefore, it is not correct to turn it off by default for all routes and turn it on for only one route, because that would be opt-in.

[fritzmg]

The functionality is currently opt-out in general.

For the BackendController we want to disable it by default and only enable it for one specific route - the contao_backend route - which is what this PR did originally. And this will work regardless of whether the feature is opt-out or opt-in by default.

With your change the latter is not the case anymore. If the feature is, for whatever reason, switched to opt-in, the contao_backend route will not store its referrer anymore in the current state.

With the original state of this PR you would not have to worry about this.

[leofeyer]

If you want it to be opt-in, then please implement it accordingly. It should be true === $request->attributes->get('_store_referrer') instead of false !== … then.

[fritzmg]

If you want it to be opt-in, then please implement it accordingly. It should be true === $request->attributes->get('_store_referrer') instead of false !== … then.

That's not what I meant. The implementation I did was opt-out, and deliberately so (see description above and discussion in the other issue).

My point was that the BackendController does not need to be concerned about the implementation detail of whether it is implemented in an opt-in or opt-out way. Nor does it need to be concerned about the way the opt-in or opt-out logic is implemented in the respective listener.

The point is that we want to ensure that none of the BackendController routes store the referrer, except for the contao_backend route. The original state of this PR would ensure this and as a bonus it would ensure this regardless of whether the actual implementation is opt-in or opt-out.

[fritzmg]

The same applies to the _token_check attribute for example. While _token_check is true by default for the frontend and backend _scope, we still set it deliberately to false or true depending on what the route or the controller's defaults need.

The controller/route definition should not care about the default handling of an attribute, if it wants to ensure something. You only need to be concerned about being able to enable or disable the CSRF token check, if you use _token_check = true or _token_check = false. Likewise you do only need to be concerned being able to enable or disable storing the back end referrer, when you use _store_referrer = true or _store_referrer = false.

[fritzmg]

Or look at it this way: let's say we release the current state in Contao 4.13.44 and 5.3.9 respectively. And then it turns out that having this enabled by default for all backend routes breaks too many extensions somehow, so we decide to flip it to opt-in instead for the next bugfix release. With your last changes you now have to remember to also adjust the contao_backend route and not just the default behavior in the listener.

With the previous state of this PR this would not be necessary.

It's about defining things defensively in a forward compatible way and keeping the concerns separate to avoid possible future pitfalls.

And my assumption is that @Toflar thought about this similarly when implementing 92ba02e for Contao 4.1.0.

[leofeyer]

I have no problems implementing this as an opt-in! But the pull request has implemented it as an opt-out, so just create a new PR and change the implementation.

[fritzmg]

You are misunderstanding the point. I do not want this to be opt-in. The recent discussion is about your change which I disagree with (see my comments).

[leofeyer]

You did not implement the opt-out pattern correctly, so I fixed it. 🤷‍♂️

[fritzmg]

Whether the attribute is opt-in or opt-out is solely defined by the implementation in the respective listener. It is not defined by the route definition itself.