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

+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 last submitted patch, 9: test-only.patch, failed testing. View results →

The last submitted patch, 9: test-only.patch, failed testing. View results →

The last submitted patch, 9: 3332546-9-comment_selection_entityqueryalter.patch, failed testing. View results →

FWIW, this addresses the original case that I reported, but IMHO it doesn't fully address the additional case we discussed when referencing a comment of a node that is unpublished and we don't have test coverage for that yet.

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.

> * As a user without "bypass node access" permission, I cannot even set the value of a comment of an unpublished node.

Yes, but that is that assuming that you are using the UI, and you changed the test to use an options list, at this point you're not testing entity validation, you're testing that the select doesn't initially contain that option.

But entity validation must work on its own as well, for example when using REST/JSON:API.

I adjusted your test to verify the validation API directly instead of using the UI, just for that method. I think it would probably make more sense for this to be a kernel test and always use the API, but I just made the minimal required changes for now. This incorrectly doesn't cause any validations in 9.5 and I expect it to do so in 9.4.

The last submitted patch, 23: comment-reference-3332546-23-test-only.patch, failed testing. View results →

Looks ready, NR as failure from test-only patch

I should have uploaded a complete patch for 9.5, it is a test only patch, but the point is that it's still failing now with that change on 9.5 and passes on 9.4.

I did outline a possible solution in our slack discussions, 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. That means we'll also need another test like the current one but where the comments are attached to entity_test or so and not a node.

And as a fallback if we don't have a target bundles, we'd need to explicitly check access on the comments being validated. Despite the other method already doing that as well, we still need the query join as there might only be a few accessible comments out of many.

@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.

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

Fair enough, to be very correct, it could be multiple bundles as long as they share the same host entity type, that's what we can only support one of.

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

Not in this function, but we have them in validateReferenceableEntities(). But loading them and checking access is slow, so I think we should only do that if we have to (like getReferenceableEntities() could also be optimized to do so).

@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.

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.

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