Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
The shortcut overview page (provided by field UI) fatals atm:
Twig_Error_Runtime: An exception has been thrown during the rendering of a template ("Route "field_ui.overview_shortcut" does not exist.") in "core/modules/system/templates/links.html.twig" at line 50. in Twig_Template->displayWithErrorHandling() (line 291 of core/vendor/twig/twig/lib/Twig/Template.php).
Proposed resolution
There is no field_ui_base_route key in the annotation for Shortcut entity.
Remaining tasks
Add some tests. That's a bad error.
User interface changes
Working UI again... ?
API changes
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | 2346969-15.patch | 872 bytes | damiankloip |
| #7 | 2346969-7.patch | 1.42 KB | Berdir |
| #6 | 2346969-6.patch | 1.43 KB | Berdir |
| #6 | 2346969-6-test-only.patch | 582 bytes | Berdir |
| #3 | 2346969-2.patch | 707 bytes | damiankloip |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedComment #2
dawehnerIt would be really great to have a test coverage here, but yeah this is critical, certainly!
Comment #3
damiankloip CreditAttribution: damiankloip commentedSpoke to berdir, the correct fix if to check this entity type key in the field_ui_entity_operations() hook.
Sorry dawehner, you need to re-look!
Comment #4
BerdirYeah, someone already "fixed" Shortcut at some point before, that's why it had that annotation but was not fieldable, which made no sense. So I removed that last night, re-exposing the actual bug.
Leaving it up for core committers to decide if this can go in without a test (we can keep this open to add tests), as it should absolutely go in before beta.
Comment #5
BerdirNo, this is wrong, sorry, did not look correctly.
Comment #6
BerdirThis should be OK.
Comment #7
BerdirHow hard can it be?
Comment #8
damiankloip CreditAttribution: damiankloip commentedMe and berdir just talked about this small patch in great detail. This is good!
Comment #12
BerdirSo the attempt to test it didn't work. That's because the test user doesn't have the relevant permission.
Which is interesting, because those permissions don't even exist, as the permission generation already has the right check. Which also means that this bug only exists for uid 1, which has any permission, even those that don't exist. So I'm really not sure if it's worth to test this?
I'd suggest to just commit the patch without the test addition...
Comment #13
alexpottAgree - no need for tests. Using the permission system as the only way to decide whether or not to add links is wrong.
Comment #14
webchickCatch suggested a follow-up though that if you call
$account->hasPermission('some non-existent permission')you ought to get an error, and we could add a test for that.Comment #15
damiankloip CreditAttribution: damiankloip commentedAgreed. Lets' just fix this page.
Twig will catch all exceptions, so throwing an exception from checking permissions would certainly be more useful for a developer. Otherwise you will see Twig_Error_Runtime, which can be misleading for people. The only trouble is that we only check if the permission exists in the array of permissions for a role. So we have no notion right now of a non-existent permission vs a permission the user does not have. This could be expensive and/or cumbersome to keep this list of all permissions.
Here is just the fix.
Comment #16
BerdirYes, to validate permissions, we would have to load them at runtime, which is something we currently avoid. Doing that would mean to cache them, and doing that would mean that we need to worry about cache invalidation. Something that we removed from testing (simpletest does validation and cached permissions for that) a while ago because it was so confusing.
Comment #17
alexpott@catch and I discussed the exception on non existing permissions and came to the same conclusion as @Berdir - totally not worth doing. Dynamic permissions that might not exist should not be used in the way they are currently in
field_ui_entity_operation().Comment #18
damiankloip CreditAttribution: damiankloip commentedYes, totally agreed. Maintaining some list of available permissions is not that cool. That is what I was implying in #15, just saying it badly :)
Comment #19
webchickOk cool. :) Thanks for looking into it. And thanks for QA testing Drupal 8! :D
Committed and pushed to 8.x. Thanks!