Problem/Motivation
Just like #2344151: Comment field access doesn't work if $items isn't present, the function incorrectly assumes that $items is always present. Critical because fatal error and I believe also related to/blocking #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency.
Proposed resolution
Looks like there is no reason to use $items, we just need the entity type and bundle, we can get this from the field definition.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#26 | language-entity-field-access-2404739-26.patch | 2.7 KB | plach |
#24 | language-entity-field-access-2404739-24.patch | 2.7 KB | pfrenssen |
#24 | language-entity-field-access-2404739-24-test_only.patch | 1.14 KB | pfrenssen |
#24 | interdiff.txt | 1003 bytes | pfrenssen |
Comments
Comment #1
BerdirUntested.
Comment #2
dawehnerThis itself looks perfect, but needs tests for sure.
Comment #4
BerdirAw.
Comment #5
plachLooks good, thanks. Just curious: when can it happen that we have no items?
I cannot imagine the language field being a bundle field :)
Comment #6
dawehnerThis happens always if views tries a general access checking for fields, quote from Field.php:
Comment #7
plachHa, Views! :)
Comment #8
plachSetting to NW for tests.
Comment #9
pfrenssenComment #10
pfrenssenComment #11
BerdirYeah, I think we don't need more than that for tests. Maybe add add an assertion for the expected return value as well?
Based on what @plach said (the language field never being a configurable field), what I did is pretty useless. I would suggest we go back to my initial patch that I never uploaded, that simply moved all that logic inside an if ($items), and adds a comment that if we have no items, then we can't decide if it is visible or not, because we do not have enough information to figure that out?
Comment #13
pfrenssenI'll have a gander.
Comment #14
pfrenssenSomething like this? I initially had a more compact version but ended up with these cascading ifs for readability. I've also expanded the documentation a bit.
Comment #15
BerdirThe conditional bundle part is no longer needed, you can use the original code instead now.
Comment #17
pfrenssenSure! Rolled back to the original approach.
Comment #20
BerdirUnrelated random fail?
Comment #22
dawehner@Berdir
UserAdminTest does not enable language module, so it should be entirely random.
Comment #23
alexpottI think we can use the original code here instead of using \Drupal as a service locator.
Comment #24
pfrenssenCertainly! By building on top of previous patches I lost affinity with the original implementation.
Comment #26
plachThanks Pieter, a surplus parenthesis slipped into the last patch ;)
Comment #28
BerdirIn general, I would disagree with #23, because $items is optional but in this case, we have no choice but to rely on it anyway. And it was the old implementation. RTBC +1.
Comment #30
alexpottThis issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 629d4cb and pushed to 8.0.x. Thanks!