Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
In #2831274: Bring Media entity module to core as Media module we want to use hook_entity_access to prevent that users can delete the source field of their media type.
While implementing that, we found this issue in the field ui. FieldConfigListBuilder::getDefaultOperations() is adding "edit" and "delete" keys in operations array, also when these operations are forbidden for the entity.
Proposed resolution
Add title attributes in the if clause
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#15 | interdiff-2836384-14-15.txt | 1.23 KB | chr.fritsch |
#15 | 2836384-15-FAIL.patch | 2.52 KB | chr.fritsch |
#15 | 2836384-15.patch | 4.02 KB | chr.fritsch |
#14 | interdiff-2836384-12-14.txt | 1014 bytes | chr.fritsch |
#14 | 2836384-14.patch | 4.84 KB | chr.fritsch |
Comments
Comment #2
chr.fritschHere is the fix
Comment #3
phenaproximaI assume this will pass the tests. Preemptively RTBC.
Comment #4
alexpottLooks like we need to add an automated test for this bug/.
Comment #5
phenaproximaHere's a test. It will fail due to the bug causing malformed variables to be passed into template_preprocess_links(), but it squarely proves the fix, because the drop buttons on any Manage Fields page should never contain any empty links.
Comment #6
phenaproximaThe fail patch didn't fail?
Comment #7
phenaproximaTrying again. The fail patch definitely fails on my localhost, yet somehow it doesn't fail on Drupal CI...possibly due to a difference in error reporting.
Comment #9
phenaproximaGOOD! That is exactly the failure I was getting, and it proves the fix. This is ready for review.
Comment #10
chr.fritschNew patch that contains the fix and the test which fails in #7
Comment #12
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commentedRerolled the patch.
Comment #13
phenaproximaVery close to done. The test needs a newline at the end of the file to appease the Git Gods, and it could do with a few comments to explain exactly what it's asserting. Otherwise, I'd mark this RTBC if I could but I contributed to the patch, so I can't :)
Comment #14
chr.fritschI fixed the newline issue and added some comments to the test.
Comment #15
chr.fritschI adjusted the patch and removed the assertions. They are not needed, because test would fail right after opening the page because of notices.
Comment #17
phenaproximaThis looks good and makes sense. The fail patch pretty well proves the fix. And it's necessary :)
Comment #18
alexpottCommitted and pushed b13cdc6 to 8.4.x and 8854fe6 to 8.3.x. Thanks!
Fixed on commit
Comment #22
Wim LeersThis forgot to remove
media_entity_operation_alter()
which has this comment:Opened #2895857: Remove media_entity_operation_alter() as planned earlier for that.
Comment #23
Wim Leers