Add a generic voter for editable table fields

[aschempp]

Follow-up to https://github.com/contao/contao/pull/4648/files#r878908541

Unfortunately, this won't work reliably, because a table can have non-excluded fields. Even our current checks e.g. for news archive would (incorrectly) return "no editable fields" even if some fields are not excluded.

I wonder why we can not-exclude fields at all, and maybe if that can/should be dropped (= all fields must be given permission in tl_user)? Also, might this lead to more post_max_size issues, and can we think of a way around that?

[Toflar]

I wonder why we can not-exclude fields at all, and maybe if that can/should be dropped (= all fields must be given permission in tl_user)?

👍 I've never really understood why I need to specify this. As a developer, I don't really care if people can exclude my field from being visible or not. I guess it was more the other way around. You would want to say if something is not relevant for permissions (like system stuff, id, pid etc.).

Also, might this lead to more post_max_size issues, and can we think of a way around that?

I think the issue is not the size of the body (post_max_size) but the number of variables submitted (checkbox fields in that case) and thus the input vars (max_input_vars). The body size is only a few KB and the default limit is 8MB.

While I agree that this is a problem for which we will need a solution for at some point, it's completely unrelated to this change. Only the checked checkboxes are submitted. So by offering more fields because they are not excluded by default anymore, nothing changes.

[aschempp]

I guess it was more the other way around. You would want to say if something is not relevant for permissions (like system stuff, id, pid etc.).

That's a good point. So there are fields that should not be excluded/not be listed as excluded. But maybe we can make a safe assumption that all fields with inputType should be excludable?

So by offering more fields because they are not excluded by default anymore, nothing changes.

but it changes the max_input_vars, which means people might run into this issue with fewer extensions than currently.

[Toflar]

but it changes the max_input_vars, which means people might run into this issue with fewer extensions than currently.

No it does not, only if you have to set more checkboxes than you have now.

[aschempp]

yeah obviously, but thats very likely since not everything is always excluded

[Toflar]

I think we could try to implement a JSON solution with JS if enabled for that. But really, it's not related to this PR. An additional extension with lots of DCA information such as Isotope has a much bigger impact than this.

[aschempp]

@contao/developers if we can agree on the general concept I'm happy to add tests 🙃

[Toflar]

I very much like the concept because it - again - moves the things to where they belong. exclude is a config. It should not be changed dynamically based on the permissions but the other way around which this PR does now. Also I think the BC break is really minor because I don't think many people relied on exclude to know if someone has permission to edit a field or not.

[Toflar]

I suppose removing all the 'exclude' => true that are now superfluous in core would be part of a separate PR as it would only pollute this one?

[aschempp]

why did you remove exclude from tl_preview_link? That is NOT CS …

[leofeyer]

Right, sorry. I can make two commits if you want, although it will be squashed anyway, so there really is no point. The PR could have been merged by now. 🤷‍♂️

[aschempp]

not in my opinion. We agreed to not have these fields excluded in 4.13 because it never makes sense to have access to the back end module but not to these fields. So why would we silently change that now?

[leofeyer]

Because the fields are supposed to be excluded by default, so the admin can hide them in the user group settings. This is the same for every other table, so I really don‘t understand why we are discussing this now?

[aschempp]

we are discussing your unrelated change to the existing behaviour. As I said, the fields are intentionally not excluded, as they were before. And I have specifically asked this question in a public call (as far as I remember) and we agreed this makes sense. So your commit silently changes existing behaviour. If you want that, please do a separate PR (and then maybe it should be fixed in 4.13 as well?) so we can separately discuss this (again)!