Comment field access doesn't work if $items isn't present [#2344151]

Problem/Motivation

Fatal PHP Fatal error: Call to a member function getEntity() on a non-object in /var/www/drupal-8/core/modules/comment/src/CommentAccessControlHandler.php on line 61

Steps to reproduce.

Create a comments view with a rest display on path /comments
Add relation to it's node with node id from url
Is preview working?
Is page in browser working?
Is curl --user admin:admin --header 'Accept: application/json' --request GET http://drupal.d8/comments
Change Row style from Entity to Fields
Repeat the checks above

Proposed resolution

Remaining tasks

User interface changes

API changes

Original report by @MartijnBraam

Got this error on a view with rest export for comments filtered by node. The view works if format is "Entity" but not with "Fields".

[Wed Sep 24 14:12:31.021783 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP Fatal error:  Call to a member function getEntity() on a non-object in /var/www/drupal-8/core/modules/comment/src/CommentAccessControlHandler.php on line 61
[Wed Sep 24 14:12:31.021880 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP Stack trace:
[Wed Sep 24 14:12:31.021918 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP   1. {main}() /var/www/drupal-8/index.php:0
[Wed Sep 24 14:12:31.021943 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP   2. Drupal\\Core\\DrupalKernel->handle() /var/www/drupal-8/index.php:22
[Wed Sep 24 14:12:31.021977 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP   3. Stack\\StackedHttpKernel->handle() /var/www/drupal-8/core/lib/Drupal/Core/DrupalKernel.php:567
[Wed Sep 24 14:12:31.021990 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP   4. Drupal\\Core\\StackMiddleware\\ReverseProxyMiddleware->handle() /var/www/drupal-8/core/vendor/stack/builder/src/Stack/StackedHttpKernel.php:23
[Wed Sep 24 14:12:31.021998 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP   5. Drupal\\Core\\StackMiddleware\\PageCache->handle() /var/www/drupal-8/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php:58
[Wed Sep 24 14:12:31.022010 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP   6. Drupal\\Core\\StackMiddleware\\KernelPreHandle->handle() /var/www/drupal-8/core/lib/Drupal/Core/StackMiddleware/PageCache.php:52
[Wed Sep 24 14:12:31.022016 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP   7. Symfony\\Component\\HttpKernel\\HttpKernel->handle() /var/www/drupal-8/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php:53
[Wed Sep 24 14:12:31.022032 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP   8. Symfony\\Component\\HttpKernel\\HttpKernel->handleRaw() /var/www/drupal-8/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:66
[Wed Sep 24 14:12:31.022046 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP   9. call_user_func_array() /var/www/drupal-8/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:126
[Wed Sep 24 14:12:31.022059 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP  10. Drupal\\views\\Routing\\ViewPageController->handle() /var/www/drupal-8/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:126
[Wed Sep 24 14:12:31.022071 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP  11. Drupal\\views\\ViewExecutable->initHandlers() /var/www/drupal-8/core/modules/views/src/Routing/ViewPageController.php:82
[Wed Sep 24 14:12:31.022083 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP  12. Drupal\\views\\ViewExecutable->_initHandler() /var/www/drupal-8/core/modules/views/src/ViewExecutable.php:787
[Wed Sep 24 14:12:31.022095 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP  13. Drupal\\field\\Plugin\\views\\field\\Field->access() /var/www/drupal-8/core/modules/views/src/ViewExecutable.php:903
[Wed Sep 24 14:12:31.022107 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP  14. Drupal\\Core\\Entity\\EntityAccessControlHandler->fieldAccess() /var/www/drupal-8/core/modules/field/src/Plugin/views/field/Field.php:217
[Wed Sep 24 14:12:31.022120 2014] [:error] [pid 13325] [client 127.0.0.1:46271] PHP  15. Drupal\\comment\\CommentAccessControlHandler->checkFieldAccess() /var/www/drupal-8/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php:297

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category	Bug because fatals
Issue priority	Critical because blocks other criticals
Disruption	None

Comment	File	Size	Author
#43	comment-rest-fatal-2344151.43.patch	17.26 KB	larowlan
#43
#41	comment-rest-fatal-2344151.39.patch	17.75 KB	larowlan
#41
#41	interdiff.txt	876 bytes	larowlan
#38	comment-rest-fatal-2344151.38.patch	17.75 KB	larowlan
#38
#38	interdiff.txt	2.67 KB	larowlan
#34	comment-rest-fatal-2344151.34.patch	15.98 KB	larowlan
#34
#34	interdiff.txt	7.24 KB	larowlan
#30	comment-rest-fatal-2344151.29.patch	16.97 KB	larowlan
#30
#30	interdiff.txt	3.5 KB	larowlan
#24	comment-rest-fatal-2344151.24.patch	16.67 KB	larowlan
#24
#24	interdiff.txt	2.96 KB	larowlan
#14	comment-rest-fatal-2344151.3.patch	20.53 KB	larowlan
#14
#14	interdiff.txt	847 bytes	larowlan
#10	interdiff.txt	9.21 KB	larowlan
#10	comment-rest-fatal-2344151.pass_.patch	20.37 KB	larowlan
#10
#8	comment-rest-fatal-2344151.fail_.patch	12.57 KB	larowlan
#8

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

clemens.tolboom

Dutch

Groningen, 🇳🇱/🇪🇺

CreditAttribution: clemens.tolboom commented 24 September 2014 at 12:25

Title:	Fatal on comment ReST export display	» Fatal on comment ReST export display using fields
Assigned:	Unassigned	» clemens.tolboom
Issue summary:	View changes
Parent issue:		» #2335229: [meta] REST et al

I vaguely remember this encounter this field related error.

Comment #2

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 24 September 2014 at 18:29

Priority:	Normal	» Major
Status:	Active	» Postponed (maintainer needs more info)

Any chance you could export the view and paste here?

Comment #3

clemens.tolboom

Dutch

Groningen, 🇳🇱/🇪🇺

CreditAttribution: clemens.tolboom commented 25 September 2014 at 07:46

@larowlan iirc @MartijnBraam added the comment body field which made it crash.

Maybe reviewing #2340471: Rest export views preview should show some output instead of 500 would help too.

Comment #4

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 25 September 2014 at 09:35

So is the error on preview or on visiting the page.
If on preview, @clemens.tolboom's issue should fix it.
If not, then it sounds like a different issue so a sample view (which could be used in a test) will be helpful.

Lee

Comment #5

clemens.tolboom

Dutch

Groningen, 🇳🇱/🇪🇺

CreditAttribution: clemens.tolboom commented 25 September 2014 at 11:37

Both preview and REST request from angular app crashed. Probably for different reasons.

Comment #6

MartijnBraam CreditAttribution: MartijnBraam commented 25 September 2014 at 11:40

This is a export of the view. The json request crashes as soon as I add the comment body field.

uuid: f15012f0-3121-4362-9b02-2de2288b070c
langcode: en
status: true
dependencies:
  entity:
    - field.storage.comment.comment_body
  module:
    - comment
    - field
    - rest
id: comments_by_node
label: 'Comments by node'
module: views
description: ''
tag: ''
base_table: comment
base_field: cid
core: 8.x
display:
  default:
    display_plugin: default
    id: default
    display_title: Master
    position: 0
    provider: views
    display_options:
      access:
        type: perm
        options:
          perm: 'access content'
        provider: user
        dependencies: {  }
      cache:
        type: none
        options: {  }
        provider: views
        dependencies: {  }
      query:
        type: views_query
        options:
          disable_sql_rewrite: false
          distinct: false
          replica: false
          query_comment: false
          query_tags: {  }
        provider: views
        dependencies: {  }
      exposed_form:
        type: basic
        options:
          submit_button: Apply
          reset_button: false
          reset_button_label: Reset
          exposed_sorts_label: 'Sort by'
          expose_sort_order: true
          sort_asc_label: Asc
          sort_desc_label: Desc
        provider: views
        dependencies: {  }
      pager:
        type: full
        options:
          items_per_page: 10
          offset: 0
          id: 0
          total_pages: null
          expose:
            items_per_page: false
            items_per_page_label: 'Items per page'
            items_per_page_options: '5, 10, 25, 50'
            items_per_page_options_all: false
            items_per_page_options_all_label: '- All -'
            offset: false
            offset_label: Offset
          tags:
            previous: '‹ previous'
            next: 'next ›'
            first: '« first'
            last: 'last »'
          quantity: 9
        provider: views
        dependencies: {  }
      style:
        type: serializer
      row:
        type: fields
        options:
          inline: {  }
          separator: ''
          hide_empty: false
          default_field_elements: true
        provider: views
        dependencies: {  }
      relationships:
        node:
          id: node
          table: comment_field_data
          field: node
          required: true
          plugin_id: standard
          provider: views
          relationship: none
          group_type: group
          admin_label: Content
          dependencies: {  }
      fields:
        subject:
          id: subject
          table: comment_field_data
          field: subject
          relationship: none
          group_type: group
          admin_label: ''
          dependencies:
            module:
              - comment
          label: ''
          exclude: false
          alter:
            alter_text: false
            text: ''
            make_link: false
            path: ''
            absolute: false
            external: false
            replace_spaces: false
            path_case: none
            trim_whitespace: false
            alt: ''
            rel: ''
            link_class: ''
            prefix: ''
            suffix: ''
            target: ''
            nl2br: false
            max_length: ''
            word_boundary: true
            ellipsis: true
            more_link: false
            more_link_text: ''
            more_link_path: ''
            strip_tags: false
            trim: false
            preserve_tags: ''
            html: false
          element_type: ''
          element_class: ''
          element_label_type: ''
          element_label_class: ''
          element_label_colon: false
          element_wrapper_type: ''
          element_wrapper_class: ''
          element_default_classes: true
          empty: ''
          hide_empty: false
          empty_zero: false
          hide_alter_empty: true
          link_to_comment: true
          link_to_entity: false
          plugin_id: comment
          provider: comment
        name:
          id: name
          table: comment_field_data
          field: name
          relationship: none
          group_type: group
          admin_label: ''
          dependencies:
            module:
              - comment
          label: ''
          exclude: false
          alter:
            alter_text: false
            text: ''
            make_link: false
            path: ''
            absolute: false
            external: false
            replace_spaces: false
            path_case: none
            trim_whitespace: false
            alt: ''
            rel: ''
            link_class: ''
            prefix: ''
            suffix: ''
            target: ''
            nl2br: false
            max_length: ''
            word_boundary: true
            ellipsis: true
            more_link: false
            more_link_text: ''
            more_link_path: ''
            strip_tags: false
            trim: false
            preserve_tags: ''
            html: false
          element_type: ''
          element_class: ''
          element_label_type: ''
          element_label_class: ''
          element_label_colon: false
          element_wrapper_type: ''
          element_wrapper_class: ''
          element_default_classes: true
          empty: ''
          hide_empty: false
          empty_zero: false
          hide_alter_empty: true
          link_to_user: false
          plugin_id: comment_username
          provider: comment
        created:
          id: created
          table: comment_field_data
          field: created
          relationship: none
          group_type: group
          admin_label: ''
          dependencies:
            module:
              - views
          label: ''
          exclude: false
          alter:
            alter_text: false
            text: ''
            make_link: false
            path: ''
            absolute: false
            external: false
            replace_spaces: false
            path_case: none
            trim_whitespace: false
            alt: ''
            rel: ''
            link_class: ''
            prefix: ''
            suffix: ''
            target: ''
            nl2br: false
            max_length: ''
            word_boundary: true
            ellipsis: true
            more_link: false
            more_link_text: ''
            more_link_path: ''
            strip_tags: false
            trim: false
            preserve_tags: ''
            html: false
          element_type: ''
          element_class: ''
          element_label_type: ''
          element_label_class: ''
          element_label_colon: false
          element_wrapper_type: ''
          element_wrapper_class: ''
          element_default_classes: true
          empty: ''
          hide_empty: false
          empty_zero: false
          hide_alter_empty: true
          date_format: long
          custom_date_format: ''
          timezone: ''
          plugin_id: date
          provider: views
        comment_body:
          id: comment_body
          table: comment__comment_body
          field: comment_body
          relationship: none
          group_type: group
          admin_label: ''
          dependencies:
            entity:
              - field.storage.comment.comment_body
            module:
              - field
          label: ''
          exclude: false
          alter:
            alter_text: false
            text: ''
            make_link: false
            path: ''
            absolute: false
            external: false
            replace_spaces: false
            path_case: none
            trim_whitespace: false
            alt: ''
            rel: ''
            link_class: ''
            prefix: ''
            suffix: ''
            target: ''
            nl2br: false
            max_length: ''
            word_boundary: true
            ellipsis: true
            more_link: false
            more_link_text: ''
            more_link_path: ''
            strip_tags: false
            trim: false
            preserve_tags: ''
            html: false
          element_type: ''
          element_class: ''
          element_label_type: ''
          element_label_class: ''
          element_label_colon: false
          element_wrapper_type: ''
          element_wrapper_class: ''
          element_default_classes: true
          empty: ''
          hide_empty: false
          empty_zero: false
          hide_alter_empty: true
          click_sort_column: value
          type: text_default
          settings: {  }
          group_column: value
          group_columns: {  }
          group_rows: true
          delta_limit: all
          delta_offset: '0'
          delta_reversed: false
          delta_first_last: false
          multi_type: separator
          separator: ', '
          field_api_classes: false
          plugin_id: field
          provider: field
      filters: {  }
      sorts: {  }
      header: {  }
      footer: {  }
      empty: {  }
      arguments:
        nid:
          id: nid
          table: node
          field: nid
          relationship: node
          group_type: group
          admin_label: ''
          dependencies:
            module:
              - views
          default_action: default
          exception:
            value: all
            title_enable: false
            title: All
          title_enable: false
          title: ''
          default_argument_type: node
          default_argument_options: {  }
          default_argument_skip_url: false
          summary_options:
            base_path: ''
            count: true
            items_per_page: 25
            override: false
          summary:
            sort_order: asc
            number_of_records: 0
            format: default_summary
          specify_validation: false
          validate:
            type: none
            fail: 'not found'
          validate_options: {  }
          break_phrase: false
          not: false
          plugin_id: numeric
          provider: views
      field_langcode: '***LANGUAGE_language_content***'
      field_langcode_add_to_query: null
  rest_export_1:
    display_plugin: rest_export
    id: rest_export_1
    display_title: 'REST export'
    position: 1
    provider: rest
    display_options:
      field_langcode: '***LANGUAGE_language_content***'
      field_langcode_add_to_query: null
      path: node/%/comments
      pager:
        type: some
        options:
          items_per_page: 10
          offset: 0
      style:
        type: serializer
        options:
          uses_fields: 0
          formats:
            json: json
      row:
        type: data_field
        options:
          field_options:
            subject:
              alias: ''
              raw_output: true
            name:
              alias: ''
              raw_output: true
        provider: rest

Comment #7

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 25 September 2014 at 11:50

Thanks, will have a look when I get a chance

Comment #8

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 25 September 2014 at 21:53

Status:

Postponed (maintainer needs more info)

» Needs review

File	Size
comment-rest-fatal-2344151.fail_.patch	12.57 KB

Fail patch, working on pass

Comment #9

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 25 September 2014 at 22:27

Here's the pass test, issue is with views passing null $items to field access during init of handler, so deferred that to the parent (As comment field access relies on having some $items) but added some checks to ensure that field-level access is still respected once $items exists.

Comment #10

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 25 September 2014 at 22:27

File	Size
comment-rest-fatal-2344151.pass_.patch	20.37 KB

interdiff.txt	9.21 KB

Comment #11

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 25 September 2014 at 22:31

Also @MartijnBraam and @clemens.tolboom note that I've changed the rest path in the view to be node/%node/comments because that is more correct.

Comment #12

andypost

he/him

Russian

CreditAttribution: andypost commented 25 September 2014 at 22:35

+++ b/core/modules/comment/src/CommentAccessControlHandler.php
@@ -57,65 +57,67 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
   protected function checkFieldAccess($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
...
+    if ($items) {

please, add inline comment why $items is null, that's really strange

Comment #13

25 September 2014 at 22:49

The last submitted patch, 8: comment-rest-fatal-2344151.fail_.patch, failed testing.

Comment #14

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 25 September 2014 at 22:52

File	Size
interdiff.txt	847 bytes
comment-rest-fatal-2344151.3.patch	20.53 KB

Added comment

Comment #15

Berdir

German

Switzerland

CreditAttribution: Berdir commented 26 September 2014 at 06:51

You should probably check $items && $items->getEntity(). I had a similar problem yesterday, but in my case, I had items but no entity when fields were displayed in a view.

This isn't "strange". $items is defined to be an optional argument, there are cases where access is checkout without having items.

This makes all the access checking optional now. Is that really what we want? Wouldn't this allow access to things like hostnames through REST, as it defaults to allowing access? Should we use default settings when we can't get them from the entity?

Comment #16

andypost

he/him

Russian

CreditAttribution: andypost commented 26 September 2014 at 07:21

I'd prefer to use default values before checking.

@Berdir

I had items but no entity when fields were displayed in a view.

is that a common case?

Comment #17

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 26 September 2014 at 08:55

@berdir - it calls access in the handler init, I thought it might also skip access checks too so added a test for both mail and host - as these are protected - neither of these were displayed - so I'm figuring it calls access again later with real items? hoping someone from views camp can confirm

Comment #18

Berdir

German

Switzerland

CreditAttribution: Berdir commented 26 September 2014 at 10:50

Not sure, all I know is that I built a field based RSS view and it blew up because I had a $items->getEntity() there that returned NULL.

Comment #19

clemens.tolboom

Dutch

Groningen, 🇳🇱/🇪🇺

CreditAttribution: clemens.tolboom commented 30 September 2014 at 10:48

Assigned:	clemens.tolboom	» Unassigned
Issue tags:		+Amsterdam2014

Comment #20

damiankloip CreditAttribution: damiankloip commented 8 October 2014 at 15:15

So this is more of a comment/possibly views module issue and not rest?

Comment #21

clemens.tolboom

Dutch

Groningen, 🇳🇱/🇪🇺

CreditAttribution: clemens.tolboom commented 8 October 2014 at 19:17

Issue summary:

View changes

I tried to reproduce the fatal view but got different results.

- comments
- fields: Title,
- relation on Node ID

I expected the preview to fail which did not broke.
Visiting the page on http://drupal.d8/comments/1 is not broken either
Running curl --user admin:admin --header 'Accept: application/json' --request GET http://drupal.d8/comments/1
is working fine.

What did I miss in reconstructing the view?

I hope to test the patch tomorrow.

Comment #22

dawehner

German

CreditAttribution: dawehner commented 14 October 2014 at 22:41

+++ b/core/modules/comment/src/CommentAccessControlHandler.php
@@ -57,65 +57,70 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
+    // Views field plugin checks access with NULL $items during handler
+    // initialization.
+    // @see Drupal\field\Plugin\views\field\Field::access()

At least the API allows so yeah +1 for ensure that you don't fail.

+++ b/core/modules/comment/tests/modules/comment_test_views/test_views/views.view.test_comment_rest.yml
@@ -0,0 +1,417 @@
+uuid: f15012f0-3121-4362-9b02-2de2288b070c

We don't add UUIDs to those test views, sorry

Comment #23

jhedstrom

English

Portland, OR

CreditAttribution: jhedstrom commented 30 October 2014 at 21:55

Status:

Needs review

» Needs work

Bouncing to needs work based on #22.

Comment #24

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 2 December 2014 at 09:43

Status:

Needs work

» Needs review

File	Size
interdiff.txt	2.96 KB
comment-rest-fatal-2344151.24.patch	16.67 KB

Re-roll and refactor to not allow null $items in the edit op.

Comment #25

dawehner

German

CreditAttribution: dawehner commented 21 December 2014 at 15:51

Status:

Needs review

» Needs work

It would be great to get this issue in, really, it add test coverage, this is quite helpful.

+++ b/core/modules/comment/src/CommentAccessControlHandler.php
@@ -57,9 +57,14 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
+        // We cannot make a decision about access to edit a field if we don't
+        // have access to the Comment entity.
+        return AccessResult::forbidden();

Isn't AccessResult::neutral() the exact tool for it? ...

+++ b/core/modules/comment/tests/modules/comment_test_views/test_views/views.view.test_comment_rest.yml
@@ -0,0 +1,416 @@
+      fields:
+        subject:
+          id: subject

note: these fields should now have 'entity_type' and 'entity_field'

Comment #26

andypost

he/him

Russian

CreditAttribution: andypost commented 27 December 2014 at 10:15

6 files were hidden/shown/deleted

File	Size
comment-rest-fatal-2344151.fail_.patch	12.57 KB

comment-rest-fatal-2344151.pass_.patch	20.37 KB

interdiff.txt	9.21 KB
interdiff.txt	847 bytes
comment-rest-fatal-2344151.3.patch	20.53 KB

interdiff.txt	2.96 KB

#25.1 Good point, but comment depends on access to parent(commented) entity access so when no entity there should be no access.

Also there's couple of nitpicks

+++ b/core/modules/comment/src/CommentAccessControlHandler.php
@@ -105,14 +110,20 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
+      $entity = NULL;
+      if ($items) {
+        $entity = $items->getEntity();
+      }

why not $entity = $items ? $items->getEntity() : NULL;

+++ b/core/modules/comment/src/Tests/Views/CommentRestExportTest.php
@@ -0,0 +1,67 @@
+namespace Drupal\comment\Tests\Views;
+use Drupal\Component\Serialization\Json;

please, add blank line between

Comment #27

dawehner

German

CreditAttribution: dawehner commented 27 December 2014 at 11:55

#25.1 Good point, but comment depends on access to parent(commented) entity access so when no entity there should be no access.

... Well, I guess things are anded together anyway.

Comment #28

dawehner

German

CreditAttribution: dawehner commented 6 January 2015 at 21:16

Priority:

Major

» Critical

Note: This fixes one of the failures of #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency, so its critical for itself.

Bump up to critical.

Comment #29

dawehner

German

CreditAttribution: dawehner commented 6 January 2015 at 22:01

Issue tags:		+VDC
Related issues:		+#2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency

Comment #30

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 6 January 2015 at 23:56

Status:

Needs work

» Needs review

File	Size
interdiff.txt	3.5 KB
comment-rest-fatal-2344151.29.patch	16.97 KB

Comment #31

kim.pepper

English

🏄‍♂️🇦🇺Sydney, Australia

CreditAttribution: kim.pepper commented 7 January 2015 at 00:03

+++ b/core/modules/comment/src/CommentAccessControlHandler.php
@@ -57,9 +57,14 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
+        // We cannot make a decision about access to edit a field if we don't
+        // have access to the Comment entity.

This comment seems more about not having any items, rather than access to the comment entity?

Comment #32

7 January 2015 at 00:17

Status:

Needs review

» Needs work

The last submitted patch, 30: comment-rest-fatal-2344151.29.patch, failed testing.

Comment #33

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 7 January 2015 at 02:09

So it fails on schema issues for the exported test view - but none of the other test views in comment module have a schema - does that mean that's an issue with rest schema?

Comment #34

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 7 January 2015 at 02:22

Status:

Needs work

» Needs review

File	Size
interdiff.txt	7.24 KB
comment-rest-fatal-2344151.34.patch	15.98 KB

#31 if we have items we can use $items->getEntity() - clarified comment

Comment #35

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 7 January 2015 at 03:11

mmmmmm green

Comment #36

andypost

he/him

Russian

CreditAttribution: andypost commented 7 January 2015 at 07:34

3 files were hidden/shown/deleted

File	Size
comment-rest-fatal-2344151.24.patch	16.67 KB

interdiff.txt	3.5 KB
comment-rest-fatal-2344151.29.patch	16.97 KB

looks great, rtbc +1

Comment #37

dawehner

German

CreditAttribution: dawehner commented 7 January 2015 at 08:42

Everyime I look at the patch I still stumble upon line #25.1

Comment #38

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 7 January 2015 at 21:24

File	Size
interdiff.txt	2.67 KB
comment-rest-fatal-2344151.38.patch	17.75 KB

Thanks @dawehner - we can be smarter about this

Comment #39

Berdir

German

Switzerland

CreditAttribution: Berdir commented 7 January 2015 at 21:27

Can we give this a bit more descriptive title and update the issue summary? I guess then it's ready.

Comment #40

kim.pepper

English

🏄‍♂️🇦🇺Sydney, Australia

CreditAttribution: kim.pepper commented 7 January 2015 at 21:28

Small nitpick:

+++ b/core/modules/comment/src/CommentAccessControlHandler.php
@@ -92,13 +85,22 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
+          // We cannot make a decision about access to edit these field if we

s/field/fields

Comment #41

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 7 January 2015 at 21:35

Title:	Fatal on comment ReST export display using fields	» Comment field access doesn't work if $items isn't present
Issue summary:	View changes

File	Size
interdiff.txt	876 bytes
comment-rest-fatal-2344151.39.patch	17.75 KB

New patch, new title, added beta eval

Comment #42

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran commented 7 January 2015 at 22:25

+++ b/sites/default/default.settings.php
@@ -250,7 +250,7 @@
- * security overrides.
+ * turning on Twig debugging and security overrides.

wrong rebase.

Comment #43

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 7 January 2015 at 22:29

File	Size
comment-rest-fatal-2344151.43.patch	17.26 KB

Comment #44

dawehner

German

CreditAttribution: dawehner commented 8 January 2015 at 00:41

Looks fine for me.

Comment #45

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran commented 8 January 2015 at 03:08

Status:

Needs review

» Reviewed & tested by the community

Let's fix it then.

Comment #46

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 8 January 2015 at 10:36

Status:

Reviewed & tested by the community

» Fixed

Committed 1c94124 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation.

Comment #47

8 January 2015 at 10:38

alexpott committed 1c94124 on 8.0.x

Issue #2344151 by larowlan: Comment field access doesn't work if $items...

Comment #48

Berdir

German

Switzerland

CreditAttribution: Berdir commented 9 January 2015 at 17:47

Comment #49

23 January 2015 at 17:54

Status:

Fixed

» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Comment field access doesn't work if $items isn't present

Problem/Motivation

Steps to reproduce.

Proposed resolution

Remaining tasks

User interface changes

API changes

Original report by @MartijnBraam

Beta phase evaluation

Comments

Parent issue

Related issues

Referenced by