Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
4.75 KB

To support menu access callbacks we seem to need still something like entity_access().

Status: Needs review » Needs work

The last submitted patch, drupal-1862754-1.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.86 KB

Oh damn, forgot to add the new file to the patch.

dawehner’s picture

Component: entity system » comment.module
FileSize
5.86 KB
520 bytes

Yeah i'm totally not sure whether it's a good step to rename 'edit' to 'update' in this patch.

Status: Needs review » Needs work

The last submitted patch, drupal-1862754-4.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.24 KB

After some discussion with fubhy let's not add another function but reuse comment_access.

Status: Needs review » Needs work

The last submitted patch, drupal-1862754-5.patch, failed testing.

Berdir’s picture

+++ b/core/modules/comment/lib/Drupal/comment/CommentAccessController.phpundefined
@@ -0,0 +1,28 @@
+  /**
+   * Overrides \Drupal\Core\Entity\EntityAccessController::updateAccess().
+   */
+  public function updateAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+    return ($account->id() && $account->id() == $entity->id() && $entity->status == COMMENT_PUBLISHED && user_access('edit own comments', $account)) || user_access('administer comments', $account);

This isn't going to end well if $account is NULL :)

Wondering if we should make $account a required argument here, to avoid having to add hardcoded assumptions about $GLOBALS['user']...

dawehner’s picture

This seems to be a good idea! I'm not sure which level should load the current user, but i guess the entity level is the only one that allows to provide a good DX.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.43 KB

We could either patch the entity class first, or implement the argument as optional and then start to optimize it.

    if (!isset($account)) {
      // The global user is stored as a stdClass so convert it into a proper user object.
      $account = entity_create('user', (array) $GLOBALS['user']);
    }

Should we maybe alternative set the global user always to an entity object?

Status: Needs review » Needs work

The last submitted patch, drupal-1862754-10.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#10: drupal-1862754-10.patch queued for re-testing.

Berdir’s picture

Should we maybe alternative set the global user always to an entity object?

Yes, but that's a different issue. with it's own set of dependencies. The global $user is set in session.inc, where we don't have access to the entity system yet. There's both an issue for doing just this and the symfony one that completely replaces the session handling.

Berdir’s picture

Status: Needs review » Needs work

Will need a re-roll and should also get an implementation for the other methods.

+++ b/core/modules/comment/comment.moduleundefined
@@ -248,7 +248,7 @@ function comment_menu() {
     'access callback' => 'comment_access',

As happened with the other conversion, let's kill comment_access() and use entity_page_access and update other access callbacks too.

+++ b/core/modules/comment/lib/Drupal/comment/CommentAccessController.phpundefined
@@ -0,0 +1,32 @@
+    if (!isset($account)) {
+      // The global user is stored as a stdClass so convert it into a proper user object.
+      $account = entity_create('user', (array) $GLOBALS['user']);
+    }
+    return ($account->id() && $account->id() == $entity->uid && $entity->status == COMMENT_PUBLISHED && user_access('edit own comments', $account)) || user_access('administer comments', $account);

This isn't really an appropriate way to create a user object IMHO.

Let's just use $account->uid and not worry about this for now.

dawehner’s picture

Status: Needs work » Needs review
FileSize
62.98 KB

Thanks for the review! Rerolled, changed the translation access controller as well.

Added a new access $op for approving comments and removed comment_access().

dawehner’s picture

FileSize
7.61 KB

I'm sorry, that's the wrong patch for this issue.

Status: Needs review » Needs work

The last submitted patch, drupal-1862754-15.patch, failed testing.

Berdir’s picture

+++ b/core/modules/comment/lib/Drupal/comment/CommentAccessController.phpundefined
@@ -0,0 +1,56 @@
+    return ($account->id() && $account->id() == $entity->uid && $entity->status == COMMENT_PUBLISHED && user_access('edit own comments', $account)) || user_access('administer comments', $account);

This should be $account->uid now :)

+++ b/core/modules/comment/lib/Drupal/comment/CommentTranslationController.phpundefined
@@ -17,23 +17,6 @@
-  public function getAccess(EntityInterface $entity, $op) {

Lovely. Did we do that for user/terms too?

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
7.61 KB

Good point.

Lovely. Did we do that for user/terms too?

For user there is no TranslationController,

One question: What should happen if you call $entity->access('random_op_that_does_not_exists'), because currently it blows up totally.

Status: Needs review » Needs work

The last submitted patch, drupal-1862754-19.patch, failed testing.

dawehner’s picture

So it fails because $account is NULL in most cases, so we somehow need to get the current user from the global.

Berdir’s picture

Oh right, missed that. Yes, but $account = $GLOBALS['user'] should be enough. That's what the other implementations do and we just check uid, which is always there (Although it will get complicated once we convert User to NG but that's not specific to this) and pass it to user_access().

dawehner’s picture

Status: Needs work » Needs review
FileSize
956 bytes
7.74 KB

Lets see whether this works.

Status: Needs review » Needs work

The last submitted patch, drupal-1862754-23.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
20.9 KB
907 bytes

Hopefully this is a better one.

Status: Needs review » Needs work

The last submitted patch, drupal-1862754-25.patch, failed testing.

Berdir’s picture

Most tests seem to fail with: Argument 1 passed to entity_page_access() must implement interface Drupal\Core\Entity\EntityInterface, string given.

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -278,6 +278,8 @@ public function build(ContainerBuilder $container) {
+    $container->register('timer', 'Drupal\Component\Utility\Timer');

Looks like you mixed this up with timer changes.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.29 KB
11.64 KB

Changed the menu definitions to %comment, no idea why we weren't doing that already as it is a nice simplifcation. Also fixed some issues in the access controller. Comment is a NG entity, so we need to use $entity->uid->value and so on.

dawehner’s picture

Thanks for fixing the remaining issues!

These changes are looking pretty solid, but I'm not sure whether I'm "allowed" to set this to RTBC?

+++ b/core/modules/comment/comment.pages.incundefined
@@ -102,26 +102,23 @@ function comment_reply(Node $node, $pid = NULL) {
+  if (!isset($token) || !drupal_valid_token($token, 'comment/' . $comment->id() . '/approve')) {

Just wondering whether we should use entity_uri() instead + append "approve"?

fago’s picture

Status: Needs review » Needs work

Just wondering whether we should use entity_uri() instead + append "approve"?

I think that would be dangerous as the entity-uri can be altered without supporting approve.

+++ b/core/modules/comment/lib/Drupal/comment/CommentAccessController.php
@@ -0,0 +1,60 @@
+  /**
+   * Implements \Drupal\Core\Entity\EntityAccessController::approveAccess().
+   */
+  public function approveAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+    return user_access('administer comments', $account);

Implements what? This does not exist. For now I think it's best to just have this method, but with an actually right comment ala "Defines access for the custom 'approve' operation." ?

For a proper solution, I think we either need to introduce a proper entity operations API or define a CommentAccessControllerInterface.

Setting to needs work for fixing the wrong comment. Else I think this is good to go.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.33 KB
13.22 KB

Let's use an interface for now.

fago’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, improvements look good. Setting to RTBC given bot gives us green light!

catch’s picture

Status: Reviewed & tested by the community » Needs review

This reverts #638070: Router loaders causing a lot of database hits for access checks which in turn is closely related to #607244: Decrease performance impact of contextual links.

The short version is that now you have to load a comment to check access, whereas in most of these cases you could just call user_access() and skip loading the comment altogether.

It's possible we could mitigate this if #1237636: Entity lazy multiple front loading happens but no-one's working on that and it wasn't a popular suggestion when I originally posted it.

Needs more discussion.

Berdir’s picture

Hm. Right, at the moment, this is a performance issue for comments. I think that it's not that much of a problem with my patch that enables static caching for comments, that should be ready.

I'll try to get some numbers of how this affects performance with and without the comments static cache issue.

catch’s picture

If we move contextual links access checks to an AJAX callback per #914382: Contextual links incompatible with render cache, you'll want to be able to check access on the comments in that separate request.

It'll still be an issue there if that access check involves loading the comments first. With #1237636: Entity lazy multiple front loading we'd instantiate the entity with the ID, but not actually load it from storage until a property is accesssed, which won't happen for the access checks.

Berdir’s picture

Status: Needs review » Needs work

I've been trying to profile this but couldn't really find those problematic access calls.

The reason seems to be that we do not display contextual links for comments. I'm not sure if we want to do that (edit.module ?) later bot for now, we seem to be ok. Couldn't find any noticable differences with/without this patch.

I did notice a few things but not related to this. In general, it seems to be very hard to profile a 300 comments page. The time it takes to render the page is *doubled* with profiling enabled, which limits the usefulness of the result a lot.

I think the main argument for consistently using $comment->access() in all cases is that it allows to overwrite comment access logic in a single place instead of having to alter all kinds of different things. And at least in the current state, it does IMHO not cost us too much.

What I did notice is that we should probably switch to $comment->access() checks in comment_links() as well, as far as possible. I think the performance decrease in that function shouldn't be high as $comment is already available. So setting back to needs work for that.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.44 KB
15.13 KB

This indeed makes the code a bit easier to read.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

That looks great, yes.

Setting back to RTBC to get some feedback to #36 from @catch or someone else.

larowlan’s picture

Any chance we can pause this until #731724: Convert comment settings into a field to make them work with CMI and non-node entities gets in (or otherwise)? Would hate another massive re-roll at this late point in time.

webchick’s picture

Status: Reviewed & tested by the community » Postponed

We can, but then let's mark this postponed.

Although that other issue is kinda controversial, so this one might be easier to get in.

Berdir’s picture

Status: Postponed » Needs review

This is the last big missing entity access conversion that is missing. I'd love to get it in as it would unblock certain quite interesting issues and other things will be broken without this (client side edit of comments once access checks are converted to entity access, entity references of comments, ..).

Berdir’s picture

#37: drupal-1862754-37.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1862754-37.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
15.07 KB

Re-roll.

andypost’s picture

Suppose better to wait for #1947892: Improve DX with EntityAccessControllerInterface

+++ b/core/modules/comment/lib/Drupal/comment/CommentAccessControllerInterface.phpundefined
@@ -0,0 +1,39 @@
+interface CommentAccessControllerInterface extends EntityAccessControllerInterface {

Any reason to implement another interface?

dawehner’s picture

Any reason to implement another interface?

The point is that we add a new method, so an extended interface seems to make sense. If you want to implement another access logic for comments, you will be happy to have an interface, which explains you what you have to implement in core.

Wim Leers’s picture

This works well! (Tested for in-place editing of comments, i.e. in combination with #1966704: In-place editing for taxonomy terms and custom blocks.)

What is blocking RTBC? The waiting for #1947892: Improve DX with EntityAccessControllerInterface?

Berdir’s picture

Doesn't matter which issue gets in first. I've no problem with re-rolling the DX issue.

This was initially postponed on the comments field issue, but I'm not seeing that much activity there and really would like to see this in as well. I don't think it would be too complicated to re-roll after this, @larowlan?

larowlan’s picture

Much of the work on the comment field issue has been taking place in #1907960: Helper issue for "Comment field"
We're stuck on some views changes that are causing some fails.
Let's not hold this patch up.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Based on #38, #40 and #49

Berdir’s picture

#44: drupal-1862754-44.patch queued for re-testing.

andypost’s picture

+1 to commit the awesome clean-up

webchick’s picture

Assigned: Unassigned » catch

Catch had some concerns earlier, want to assign it to him to make sure that he's comfortable with this.

Berdir’s picture

If necessary, I can, at least for now, roll back the menu access callback changes. My main interest right now is to make $comment->access($op) work ASAP to be able to rely on the entity access API for the main content entities and therefore allowing to remove unnecessary hooks and node-specific code like e.g. in edit.module and file download access checks.

catch’s picture

Status: Reviewed & tested by the community » Fixed

So the access callback issue isn't a problem in core at the moment, it's going to be one if we ever attach contextual links etc. to comments again though. Really that's contextual module's problem rather than comments, so I've gone ahead and committed this.

If I regret it later this comment will be linked back to in git blame for extra facepalming.

webchick’s picture

LOL. :) Sounds good. Thanks, catch!

Wim Leers’s picture

ROFL

Status: Fixed » Closed (fixed)

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