CommentSelection::entityQueryAlter() fails on validate when referencing entity is not a comment

Fixed CCF for #2.

I worked on the previous issue.

Why don’t we just change if ($comment) { to if ($comment && ($comment instanceof CommentInterface)) { and leave the rest unchanged? If we don't even have a comment in the "CommentSelection" plugin, we shouldn't go ahead with the rest of the code.

Also, the issue with hardcoding "node" is assuming that the entity will be a node, and that even the "node" module will be installed. That’s why the previous issue (and related) were initially reported. ie: #3272023: Do not check node_access if node module is not installed

This patch is more generic and doesn't fall back on the hardcoded "node". It's still a workaround and it still needs tests, so I'll mark it as "Need work".

Help with the tests specially would be appreciated but in any case, I'll try to give it a go.

I've been doing some good debugging and I am able to reproduce it on a gitpod instance and I think I have identified all the parts of the code that might need changing. I will try to get a test as well as the code. I am assigning this to myself but if somebody else wants to try it feel free to ping me.

As the MRs testing is failing, I attach it in patch format. Attaching both a test-only and a full patch.

So prior to #2958241: Impossible to reply to comments: commented entity considered unreferencable because CommentSelection::entityQueryAlter() joins on {node_field_data} table we joined against node for access checking.

That appears to be lost in the current patch?

It was mostly to optimize the query to have a smaller subset of records. The access checking happens on the "getReferenceableEntities" method that was introduced in #2958241: Impossible to reply to comments: commented entity considered unreferencable because CommentSelection::entityQueryAlter() joins on {node_field_data} table. This part of the code is run on validation, but also when querying available comments.

The issue is that in the past comments were always linked to nodes, but that's not the case anymore, as they can be linked/referenced to/by any entity, and even the same comment type could be linked to many entities, so you cannot just join those tables anymore.

However, 99% of the use cases will be comment replies (where the reply has a reference to the parent comment) and in that case, when we have the entity and it's a comment, we do the join if the entity that the comment is attached to does have the necessary tables to join (that logic was implemented in the previous issue).

So all we are doing is making sure that the 1% doesn't go through the "if", by checking the type. If it's not a comment, then don't do the joins as we have no way of knowing which comment it is (because $comment is actually the entity that has the comment reference, not the comment). It's kind of complex to explain, so I hope that it makes sense.

I was discussing these cases and options with @berdir and @catch, in case you need further context.

+1 RTBC

Works as expected and the type checking makes sense.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Removing the tests tag as they were added in #9.

Yes there was a bug with the MR composer build a few weeks ago that has been resolved.

I can confirm the bug still exists on D10.1 and that the patch in #9 does solve the issue.

The above "if" only runs when saving the data.

When a node is unpublished, then any other entity trying to reference the comment won't even get it offered thanks to this: https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/modules/com...

If the comment was already saved previously to unpublishing the node, the reference is still kept but the comment won't show in front-end due to a similar check on the comment entity. I think this is the expected and right behaviour.

I'll try to work on a test case for it anyway.

@Berdir - I've added two more tests:
* As a user with "bypass node access" permission, I can choose the value of the comment of an unpublished node and view it, but users without that permission can't view it.
* As a user without "bypass node access" permission, I cannot even set the value of a comment of an unpublished node.

Please review this. Those were the only additions. I am using the MR instead of the patch (which I'm hiding).

Note that the error reported in #17 was a false negative. The issue was marked as RTBC, but after @Berdir feedback, I guess the logical step is to mark it as "Needs review" again.

Hiding files which aren't relevant anymore.

Looks ready, NR as failure from test-only patch

@Berdir - thanks for the small code change to the test and the explanation after. I'll try to work on that approach next.

Just as a reminder (to myself?) about the conversation we had:

that involves looking at the selection handler configuration, picking the target bundles and then using their configuration and target type to do the correct join on the right host entity type.

We can only make the joins if the comments can be attached just to one bundle.

And as a fallback if we don't have a target bundles, we'd need to explicitly check access on the comments being validated

We do not have the comment being validated in the function, only the host entity. So not sure about this fallback part.

@Berdir - turns out that we do not need to load entities on "validateReferenceableEntities", so I mirrored the logic found in the other functions.
"buildEntityQuery", "validateReferenceableNewEntities", and now too "validateReferenceableEntities" filter out the unpublished nodes from the results.

Also, "entityQueryAlter" checks the type of the variable before making any additional joins.

I turned the Functional test into a Kernel one as well.

I think this covers all cases of UI and also validation API, so I'm not sure if we need to implement the additional logic related to the selection handler configuration in the "entityQueryAlter" method.

Please review and let me know if there are any additional cases that we need to cover or further things to cater for.

Confirmed this issue on D10.1

Using standard profile, added a reference field on the Article content type, referencing comments
Created an Article, added a few comments
Created a 2nd Article tried referencing one of the comments from Article A and got the fatal error.
Applied the MR and the issue is resolved.

#23 shows the tests are properly covering the change.

Left one minor request for the test

Can we also get a version of this for 11.x?

Thanks

Addressed the minor request and created an MR for 11.x: https://git.drupalcode.org/project/drupal/-/merge_requests/4072

As the change was minor and the issue was RTBC I will set it back to RTBC but if that's not the process please let me know or just change it to Needs review as agreed with @larowlan via slack.

Left a comment on the MR, I think something's not right in the test

Feel free to ping me when this is back at RTBC

Again, addressing really minor feedback, tests are green again, so back to RTBC, and will ping @larowlan via slack about this.

Which MR is for Drupal 9.5?
I think they both apply.

Left another comment about the test, same as before - fine to ping/self RTBC

@SocialNicheGuru, re #37: https://git.drupalcode.org/project/drupal/-/merge_requests/3233 is the one against 9.5.x

As agreed via slack, marking it again RTBC and pinging @larowlan.

Committed 13955d6 and pushed to 11.x. Thanks!

Backported to 10.1.x

9.5.x and 10.0.x are effectively in security only support now, so not backporting any further.

Thanks folks, especially @fjgarlin for the persistence here.