Typed Data's EntityDeriver does not derive bundle-level data types when a bundle has the same name as its entity type (f.e. entity:comment:comment)

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php
@@ -99,7 +99,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
       foreach ($this->bundleInfoService->getBundleInfo($entity_type_id) as $bundle => $bundle_info) {
-        if ($bundle !== $entity_type_id) {
+        if ($entity_type->getKey('bundle')) {

Seems like this check should now be outside of the loop. No need to call `$this->bundleInfoService->getBundleInfo($entity_type_id)` because we can evaluate `$entity_type->getKey('bundle')` before the loop since it is not going to change.

Otherwise looks good. It should be same either way but it is small performance and clarity fix

Ok looks good. Nice catch with this issue!

RTBC assuming #12 passes and #11 test only fails.

Confirmed that this patch in #12 applies cleanly to the current 8.7.x, and fixes the "entity:comment:comment" error in jsonapi_schema, exciting to see that module working!
Not marking RTBC yet though since all those patches just failed automated testing.

Kernel tests containing knowledge of internal implementation details is once again getting in the way.

Yep looks good!

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php
@@ -98,8 +98,8 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+      if ($entity_type->getKey('bundle')) {
+        foreach ($this->bundleInfoService->getBundleInfo($entity_type_id) as $bundle => $bundle_info) {

I think this is still the wrong approach. Bundles can be provided by something other than a config entity, e.g. hook_entity_bundle_info and hook_entity_bundle_info_alter.

In my opinion, the correct fix also needs to check if the count of bundleInfoService->getBundleInfo() is greater than 1.

So something like:

$bundle_info = $this->bundleInfoService->getBundleInfo($entity_type_id);
if (count($bundle_info) > 1 || $entity->getKey('bundle'))

Also, needs a test for that where we have a hook that implements entity_bundle_info - entity_test module implements this, so you should be able to leverage that

Fixed a deprecation error that went unnoticed because this was rolled against 8.7.

Addressed #24.

Thanks to @larowlan's excellent remark (🙏), we now know that the proposed resolution in the issue summary is also inaccurate: it's too simplistic. 😲

1. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDeriverTest.php
   @@ -0,0 +1,94 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   +  public static $modules = ['system', 'field', 'user', 'node', 'comment', 'entity_test'];
   

   🤓 Formatting nits here.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDeriverTest.php
   @@ -0,0 +1,94 @@
   +    $this->installEntitySchema('user');
   +    $this->installEntitySchema('node');
   +    $this->installEntitySchema('node_type');
   +    $this->installEntitySchema('comment');
   +    $this->installEntitySchema('comment_type');
   

   🤓 We don't need most of these entity schemas to be installed because we're not saving such entities.

Fixed.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDeriverTest.php
@@ -0,0 +1,95 @@
+    entity_test_create_bundle('foo', NULL, 'entity_test_no_bundle');
+    entity_test_create_bundle('entity_test_no_bundle', NULL, 'entity_test_no_bundle');

FYI entity_test_no_bundle was specifically chosen because it does not list a bundle entity key! This is what @larowlan was getting at in #24. Took me some time to grok 😇

Something went wrong with that patch @gabesullice, patch size dropped dramatically and it no-longer applies

error: patch failed: core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php:105
error: core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php: patch does not apply
error: core/tests/Drupal/KernelTests/Core/Entity/EntityDeriverTest.php: does not exist in index
make: *** [patch88] Error 1

Committed 6d3b05c and pushed to 8.8.x. Thanks!

🎧 this commit was brought to you by henchlock by thee oh sees

C/p as 8adf90654d and pushed to 8.7.x

Rolled this back on 8.7.x as it broke php 5.5 testing, expectException does not exist in the version of phpunit we use on php 5.

See https://www.drupal.org/pift-ci-job/1381558

#42's fix looks good and it passed PHP 5 testing so 👍

Committed e731e88 and pushed to 8.7.x. Thanks!

Added #3077607: Entity types w/o bundle key should not be able to define multiple bundles as related.

I have a question about this topic regarding the documentation on \Drupal\Core\Entity\EntityAccessControlHandler::checkCreateAccess and friends.

The documentation is as follows (trimmed for brevity):

* @param string|null $entity_bundle
* (optional) The bundle of the entity. Required if the entity supports
* bundles, defaults to NULL otherwise.
*
* @return \Drupal\Core\Access\AccessResultInterface
* The access result.
*/
protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {

It appears that after this has landed, the result of \Drupal\Core\Entity\EntityTypeInterface::getBundleEntityType is no longer respected to determine whether or not an entity has bundles or not.

As a result, instead of NULL being passed to the access control handler, it's passing a default bundle that's the same as the entity type id.

Is this a BC concern? We've had one minor breakage from what appears to have resulted from this.

If not, should we update the documentation within the EntityAccessControlHandler?

Thanks :)

Good evening,

I'll try to provide exact reproduction details tonight without any of our custom code in place on a fresh 8.7 installation.

If this doesn't happen with the core entity types (I'll be targeting the User entity) then I'll just chalk it up to a quirk somewhere else in our codebase. In either case I'll let you know.

Thanks

Hello,

After tracing things out more thoroughly, it actually seems that https://www.drupal.org/node/3038254 may be a more probable avenue to explain the change that I am seeing.

Here are the steps that I took to reproduce:

1) Install Drupal 8.7.7
2) Enable the media library module
3) Create an image media type
4) Add an image.
5) Create an entity reference field on the user entity to a media item
6) Go to the user add form and open the media library widget and select a media item, but do not insert it yet.
7) Set a breakpoint at line 284 in EntityAccessControlHandler.php
8) Click insert to pause at the breakpoint in the XHR request.
9) Observe that $context['entity_type_id'] is 'user' and that $bundle is also 'user'.

I completely missed that change record earlier. Sorry about any confusion that I may have caused here.

I'll follow up with the media team.

Thanks again

Per #52.