Problem/Motivation

In Drupal 7, fields were shared across all entity types (eg, field_foo could be attached to a node, and to a user). Thus, this module by only storing the field name was in keeping with how core operated with regards to fields.

Now however, in Drupal 8, fields are stored per entity type, so the way this module is working means if field_foo has field permissions for nodes, and then another field of the same name is added to the user entity, permissions are inherited from the node, and cannot be changed for the user (furthermore, changing the permissions on the user field actually changes them for the node too).

Proposed resolution

Store the entity machine name as part of the permission's machine name.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#20 Screen Shot 2022-06-08 at 19.36.19.png9.03 KBosopolar
#10 2825905-10.patch14.26 KBjhedstrom
#10 interdiff-2825905-08-10.txt10.18 KBjhedstrom
#8 2825905-08.patch4.08 KBjhedstrom
#8 interdiff-2825905-05-08.txt811 bytesjhedstrom
#5 2825905-04.patch4.07 KBjhedstrom
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom created an issue. See original summary.

dmsmidt’s picture

dmsmidt
he/him
Primary language Dutch
Location Amsterdam 🇳🇱🇪🇺
CreditAttribution: dmsmidt at Triquanta commented
Title: Field permissions are being shared across entity types » Field permissions are being shared across entity types and bundles
Priority: Major » Critical

This is indeed problematic.
I notice that if you have a certain field with instances on multiple bundles the settings are also shared.
So if we make these settings a per instance setting, both problems would be solved.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Priority: Critical » Major

I think this needs some re-consideration. Given that the current behavior is exactly how Drupal 7 worked (per-storage, not per-instance), this is potentially even a feature request.

If we were to change this to per-instance settings, for a single field, say used on 5 bundles, this change would suddenly result in 25 individual permissions, which makes management 5 times as much work as it is now (and 5 times as error prone in making a mistake in permissions). Multiply this for a site with 50 or 100 fields...

This might need to be targeted for a 2.x version of the module...

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Title: Field permissions are being shared across entity types and bundles » Field permissions are being shared across entity types

I split the per-bundle bit off into a new feature request: #2881776: Implement field permissions per-bundle (field instance).

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Active » Needs review
Issue tags: -beta blocker +Release blocker
FileSize
2825905-04.patch4.07 KB

Here's a really rough start.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Issue tags: +Needs update path

Status: Needs review » Needs work

The last submitted patch, 5: 2825905-04.patch, failed testing.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs work » Needs review
FileSize
interdiff-2825905-05-08.txt811 bytes
2825905-08.patch4.08 KB

This fixes some of the failures.

Status: Needs review » Needs work

The last submitted patch, 8: 2825905-08.patch, failed testing.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs work » Needs review
FileSize
interdiff-2825905-08-10.txt10.18 KB
2825905-10.patch14.26 KB

This fixes the tests.

We'll still need an upgrade path here (with tests I think).

dmsmidt’s picture

dmsmidt
he/him
Primary language Dutch
Location Amsterdam 🇳🇱🇪🇺
CreditAttribution: dmsmidt commented

Thanks for working on this, I have a bunch of other issues to review and work on, but I hope I can check it later.

sgurlt’s picture

sgurlt CreditAttribution: sgurlt commented

Looks pretty good for now, we are using this in our current development project.
Thanks for the work !

Edit: Just ran into a bug.
I have two paragraph types that share a field. One field is restricted by permissions to be used, but just for one of the paragraphs. But it seems like the permissions are effecting both paragraphs types.

pifagor’s picture

pifagor
Location 🇺🇦 Rivne
CreditAttribution: pifagor at GOLEMS GABB for GOLEMS GABB commented
Status: Needs review » Reviewed & tested by the community
achton’s picture

achton
Location Denmark
CreditAttribution: achton at Reload commented
Status: Reviewed & tested by the community » Needs review

@pifagor Did you see the bug report from @sgurit? I don't think we can RTBC this without adressing that report.

pifagor’s picture

pifagor
Location 🇺🇦 Rivne
CreditAttribution: pifagor at GOLEMS GABB for GOLEMS GABB commented
Status: Needs review » Needs work

Hello @achton. Yes, of course, you are right. I just did not come across this error.

littletiger’s picture

littletiger CreditAttribution: littletiger commented

hi all, what's the current status on this? Need help testing and reviewing or is it waiting for another bug?
There's one mentioned but not linked

cslevy’s picture

cslevy CreditAttribution: cslevy commented

Hi,
Also some update script should be provided for the existing permissions. I applied the patch, and broke the permissions on all fields.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at MachbarMacher commented
Status: Needs work » Postponed
Related issues: +#2881776: Implement field permissions per-bundle (field instance)

Reading the patch in #2881776: Implement field permissions per-bundle (field instance), this seems to be included there. Postponing on that.

berenddeboer’s picture

berenddeboer CreditAttribution: berenddeboer at Xplain Hosting commented

Big surprise to find this. At minimum a warning should be displayed, until this can be fixed. Else you change one field, and everywhere else you break things.

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
CreditAttribution: osopolar as a volunteer commented
FileSize
Screen Shot 2022-06-08 at 19.36.19.png9.03 KB

I also was very surprised to run into this issue. It's a very unexpected behavior. The field settings say (in tiny letter):

These permissions apply to all instances of this field.

EDIT:

Anyway, the patch from #2881776: Implement field permissions per-bundle (field instance) fixes the issue for me.