Improve \Drupal\content_moderation\Plugin\WorkflowType\ContentModeration::appliesToEntityTypeAndBundle() and ::getBundlesForEntityType()

Crossposting my review on this change from the other issue:

So this change made me question why we would be looking up the bundles for an entity type that's not used for a workflow, or if there's a valid scenario for an entity type that is used for a workflow to have an empty list of bundles. The reason for my concern was that this is changing the method to silently return an empty array if the entity type ID is not valid. In HEAD, this method is only used within a foreach of getEntityTypes() so it is impossible that the entity type would not be valid. The method appliesToEntityTypeAndBundle() already exists, so it seemed to me that this API change was unnecessary. Instead, the calling code could just check appliesToEntityTypeAndBundle() before calling this method. @alexpott made the case that this API change makes the calling code more robust by always returning an array.

I also wondered if a developer would instead expect an explicit failure (exception, FALSE return value, or whatever) from this method for an unsupported entity type. @alexpott pointed out that \Drupal\field\Entity\FieldConfig::__construct() (for example) doesn't validate that the bundle matches the entity type, so maybe the rest of core does not let developers expect such explicit failures.

So TLDR, this change makes the DX more friendly and there are precedents elsewhere in core for simply returning an empty array if the entity type or entity/bundle combo are not relevant in context.

Also just tagging for clarity. @timmillwood also implicitly signed off on this change from the other issue but would still be good to have an explicit signoff on this as a design behavior. Thanks!

Lgtm

Giving explicit sign off.

As @xjm said, returning an empty array is a familiar approach in core.

+++ b/core/modules/content_moderation/tests/src/Kernel/ContentModertaionWorkflowTypeApiTest.php
@@ -0,0 +1,77 @@
+  public function testGetBundlesForEntityType() {
+    /** @var \Drupal\content_moderation\Plugin\WorkflowType\ContentModeration $workflow_plugin */
+    $workflow_plugin = $this->workflow->getTypePlugin();
+    $this->assertEquals([], $workflow_plugin->getBundlesForEntityType('node'));
+    $workflow_plugin->addEntityTypeAndBundle('node', 'page');
+    $this->assertEquals(['page'], $workflow_plugin->getBundlesForEntityType('node'));
+    $this->assertEquals([], $workflow_plugin->getBundlesForEntityType('block'));
+    $workflow_plugin->removeEntityTypeAndBundle('node', 'page');
+    $this->assertEquals([], $workflow_plugin->getBundlesForEntityType('node'));

+1 for these explicit tests of the conditions, especially the empty cases.

Small improvement I suggest:

xjm [1:29 PM]
@alexpott One small thing on the tests for the blocker issue... part of what we are testing is that this part of the API does not care whether these are valid entity type/bundle relationships. But by using the names of existing entity types and bundles, one might mistakenly assume that the test is testing it for those existing ones. Maybe we could call them fake_node, fake_page, etc. or something?

[1:29]
Sorry, didn't think of this til now

alexpott [1:30 PM]
xjm: np - I debated what values to use in my head - we’re in a kernel test so node, block all don’t exist but on second thought it is clearer if these aren’t real so any false expectation it not built up.

xjm [1:31 PM]
@alexpott yep exactly; I had to double-check that the kernel test was not installing node and block schemata

We can also document clearly that this API does not validate the entity type/bundle relationship or even that they exist, just whether they are configured for the workflow and the list will be empty regardless of why they don't.

I debated whether to add a CR but I don't think we need one since it's changing a return value from a bad, undocumented NULL + warning to an expected empty value.

Interdiff seems to cover everything.

Great! This full patch, with explicit docs and tests for the related methods, makes the API make a lot more sense to me.

Since this is an API hardening for an alpha experimental module, I've also backported this change to 8.3.x. Thanks!

Slightly overeager I guess: #2850592: Typos in ContentModertaionWorkflowTypeApiTest

Oops guess I read what I want to see.