Throw a better exception when a reference field can't find the target entity type

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -4,12 +4,14 @@
+use Drupal\Component\Plugin\Exception\PluginNotFoundException;

While I realise that this hasn't made it into official coding standards yet, isn't it good practice to alphabetise the use statements?

https://www.drupal.org/project/coding_standards/issues/1624564

Otherwise, good job. 👍

Nice quality of life fix, considerably more useful than existing. Thanks.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -114,7 +116,17 @@ public static function mainPropertyName() {
-    $target_type_info = \Drupal::entityTypeManager()->getDefinition($target_type);
+    try {
+      $target_type_info = \Drupal::entityTypeManager()->getDefinition($target_type);
+    }
+    catch (PluginNotFoundException $e) {
+      throw new FieldException(sprintf("Field '%s' on entity type '%s' references a target entity type '%s' which does not exist.",
+        $field_definition->getName(),

Minor but it would be good to add a @throws to the phpdoc. However, it's currently doing @inheritdoc and I'm not sure it's worth breaking that to add the one line... what do you think?

#8 is reasonable - let's open a follow-up to improve the ::schema() docs.

Looks like it ought to be possible to add some test coverage here.

Fixed CS errors.

Could still use some test coverage.

Kindly review patch for version 9.5x,,

I will work on this.

I have resolved the patch #17 indentation issues.
Please review.

Patch #13 applies fine to 9.5.x, there was no reason for the reroll in #17. The only difference is changed indentations that fail PHP coding standards. Removing credit.

Can I please clarify: you're removing credit from everyone that hasn't submitted what you deem a relevant patch?

I understand that we need to be careful of people gaming the system, however all too often I see reviewers and commenters on issues that don't receive credit (yes, myself included on this issue).

Arguably a patch that does nothing but introduce errors without moving the code forwards should be considered, but have the contributions that Catch and I made not been valuable to the conversation?

I believe that this can only harm the community as there is a chance that people may not continue reviewing if they're not recognised for it.

Can I please clarify: you're removing credit from everyone that hasn't submitted what you deem a relevant patch?

I will remove credit for patches that claim to be rerolls when a reroll wasn't actually needed. I suppose that means I don't "deem it relevant", but that phrasing makes it seem like a more subjective decision than it is. #17 claims to be a reroll of #13, despite that patch not needing a reroll. The testbot proves that the patch in #13 applies to 9.5 with all tests passing, so the reroll that followed it was clearly not needed.

I understand that we need to be careful of people gaming the system, however all too often I see reviewers and commenters on issues that don't receive credit (yes, myself included on this issue).

Credit isn't actually granted until an issue is committed, and committers will make sure to add anyone who left useful comments. The state of the credit checkboxes before a commit is not indicative of anyone being denied credit.

Committers rarely miss adding people who added productive comments before the commit. Because providing an attachment automatically switches the credit checkbox "on" for that user, and the uploads can look similar to legitimate contributions, it's easier for a committer to miss the not-legitimate contributions. I preemptively remove credit so ensure it's not missed during the commit process, and so the user is aware they're not properly contributing to the issue so they can change their behavior moving forward.

Arguably a patch that does nothing but introduce errors without moving the code forwards should be considered, but have the contributions that Catch and I made not been valuable to the conversation?

I didn't deny you or Catch credit. Is there a wording in #23 that would have made it clearer that this was specific to just the user that uploaded an unnecessary reroll?

Thank you for the clarification, appreciate your response.

Credit isn't actually granted until an issue is committed, and committers will make sure to add anyone who left useful comments.

Committers rarely miss adding people who added productive comments before the commit.

This has not been my experience, which is why I raised the question about the comments and reviews that have been left so far. In my experience people that don't add patches are more often than not omitted from being credited. I realise that without a patch people are not automatically added to the credit list, however I (incorrectly it seems) assumed that if you were doing a tidy up in the credits that the list might have changed. I apologise if I've misconstrued.

For those of us that aren't able to spend as much time in the issue queues as we'd like, the odd credit here and there does have meaning and I've missed out on a good amount of credits when not adding patches.

Thanks for your time, appreciate it.

@kiwimind If you (or anyone reading this, TBH) see a completed issue you believe you should have received credit on, you can contact the committer on Drupal Slack. There's a chance it was a mistake, and even if it wasn't they can provide additional explanation. As careful as I try to be when committing, I can sometimes miss someone, so I appreciate when someone alerts me to that.

Thank you for your gracious reply. Realised I've hijacked this issue now, apologies.

I'll bear that in mind for future reference.

All the best.

Added patch against #22 in Drupal 10.1.x

Can confirm that #34 is a copy of the MR minus the index numbers.

But tested the MR.
The tests fail without the fix and pass with the fix.
Looks like a great update to have and will be useful!

Putting back as the MR was reviewed not the patch

Removing credit for the unnecessary patch in #34.

Looking back, we haven't had a clean test-only run proving the test coverage. I'm opening a separate MR branch for that.

Saving issue credits as follows according to the core issue credit guidelines:

• @joachim, for a useful original issue report and work on the patch
• @sourabhjain, for a necessary coding standards fix
• @ranjith_kumar_k_u, also for a necessary coding standards fix
• @kiwimind for a useful suggestion to alphabetize the use statements, which is indeed a sort-of-coding-standard for core and a point of feedback I would have given myself
• @catch, for code review
• @bnjmnm, for mentoring

This looks pretty good, except we also need to update the @throws documentation for the method. Thanks everyone!

Removing credit from myself as I just rebased to see if the tests passed, will let the committer decide to add credit back :)

The changes look good to me. The changes requested by @xjm to be addressed.

Unfortunately this seems to have been bitten by the random composer error so the tests won't run.

Uploading a patch for the tests to run

For any failures please do not reroll this issue is being worked on in the MR this is just for tests.

Seeing green.

Not sure this is blocked until the composer issue is resolved.

I wonder if we should have a change record for this? Someone could have code handling the old plugin exception.

Took a crack at the CR.

Make a few small edits to the CR.

This looks good to me now, thanks! Committed to 10.1.x and published the CR. Since a change to a thrown exception is a disruptive change, I did not backport it to the production branches.

@smustgrave @xjm Sorry to be nitpicky and pedantic, but don’t you need a comma before “which” in the error message?
https://www.thepunctuationguide.com/comma.html#settingoffnonrestrictivei...