throw an exception when IDs passed to loadMultiple() are badly formed

Followed @joachim suggestion since seemed good for me. Just changed $bad_ids to $invalid_ids that I felt was more clear. Please review.

Fix looks good. Now it needs a test.

@joachim: Thank you for adding the testing.

Just a couple of nitpicks and than it is RTBC for me.

The change look good to me.
The fix has testing for it.
The new exception has documentation.
For me it is RTBC.

@joachim: Only the person who creates the MR is allowed to mark threads as resolved at the moment. I am just working with the tools that we have.

Not really sure about throwing an exception here. Loading entities is something that is done A LOT in custom and contrib code. As mentioned in #3037956: Improve DX of load / loadMultiple / loadRevisionload / loadMultipleRevisions:

While you could argue the calling code should not call the method with "garbage" input, this means that calling code always needs to do 2 checks:
1. Is the input a string or int.
2. Is the return value of the specific load method not NULL.

It would be nice if you could call load() / loadMultiple() / loadRevision() / loadMultipleRevisions() without having to check the input, and let the load methods return NULL if it can't find an entity. I've seen methods (like ContentEntityStorageBase::getLatestTranslationAffectedRevisionId()) that could return a entity ID or NULL and I think the DX would be a bit better if you don't have to do the extra check.

Throwing an exception here can only change in a major release - there is plenty of contrib and custom code out there resulting in this error, which will result in fatal errors on production sites if we just change it in a minor.

However we can prepare to throw the exception and log a more useful error message (and maybe a backtrace) in the meantime.

Also pretty sure there are old issues for this around.

We can do a @trigger_error(), just like a deprecation message that in D10 the message will be replaced by an exception.

Yes exactly what @daffie said (and also the second half of my sentence).

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Edge-case:

I have 0, 1 or 2 entities to load and the order matters in my case.
For performance reasons I want to load them `loadMultiple()`

$nodeIds = [
  $queryPrev->execute()->fetchField(),
  $queryNext->execute()->fetchField(),
];
$nodes = $this->entityTypeManager->getStorage('node')->loadMultiple($nodeIds);
return [
  '#prev' => !empty($nodes[$nodeIds[0]]) ? $nodes[$nodeIds[0]] : NULL,
  '#next' => !empty($nodes[$nodeIds[1]]) ? $nodes[$nodeIds[1]] : NULL,
];

To avoid the warning you need extensive logic. E.g:

if (count(array_filter($nodeIds)) === 2) {
  $nodes = $this->entityTypeManager->getStorage('node')->loadMultiple($nodeIds);
}
elseif (!empty($nodeIds[0])) {
  $nodes = [
    $this->entityTypeManager->getStorage('node')->load($nodeIds[0]),
    NULL,
  ];
}
elseif (!empty($nodeIds[1])) {
  $nodes = [
    NULL,
    $this->entityTypeManager->getStorage('node')->load($nodeIds[1]),
  ];
}

But if you would have 3, 4, 5 entities to load multiple single loads would be needed.

I'm not demanding changes, just wanted to point out to this DX

Rerolled the patch for Drupal 10.2.x

Current development branch is 11.x and changes should be in MRs

Removing the patch as we already have the patch here https://www.drupal.org/files/issues/2023-01-12/3145190-21.patch