Problem/Motivation
While working on JSON:API Schema, which iterates over data types to derive a schema, this error occurs when comment
module is installed:
[error] The "entity:comment:comment" plugin does not exist. Valid plugin IDs for Drupal\Core\TypedData\TypedDataManager are: {long list of data types...}
After some digging, it appears it's because EntityDeriver.php
has this guard:
if ($bundle !== $entity_type_id) {
$this->derivatives[$entity_type_id . ':' . $bundle] = [ ...
This is a problem because bundles can have the same ID as their associated entity type ID. This happens in core when the comment
module is installed because of the "Default comment" bundle which has a machine name of comment
.
Proposed resolution
Change the guard to check that the entity type does not have a bundle entity type and also to check that a bundle has not been defined by hook_entity_bundle_info(_alter)
rather than the simple check that the bundle ID does not equal the entity type ID.
Remaining tasks
Supply a patch.
Write a test.
Review
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
None.
Comment | File | Size | Author |
---|---|---|---|
#3 | 3061610-2-test-only.patch | 2.5 KB | gabesullice |
#3 | 3061610-2.patch | 3.33 KB | gabesullice |
#7 | interdiff.txt | 853 bytes | gabesullice |
#7 | 3061610-2-test-only.patch | 2.5 KB | gabesullice |
#7 | 3061610-7.patch | 3.32 KB | gabesullice |
Comments
Comment #2
gabesulliceComment #3
gabesulliceComment #4
gabesulliceComment #7
gabesulliceI couldn't figure out why those two unrelated tests were failing and I spoke to @tedbow. He reminded me that bundles don't have to be entities, which made me realize that
$entity_type->getKey('bundle')
would work better than$entity_type->getBundleEntityType()
for this. Hopefully he can be given issue credit for that help :)Re-uploading the test only patch for readability.
Comment #9
gabesulliceAdded some missing comments.
Comment #10
tedbowSeems 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
Comment #11
gabesulliceForest, trees... meet Gabe.
Good call @tedbow!
Comment #12
gabesulliceThe #11 patch had this indentation change in it, even though it wasn't in the interdiff. Fixed.
Comment #13
tedbowOk looks good. Nice catch with this issue!
RTBC assuming #12 passes and #11 test only fails.
Comment #18
zrpnrConfirmed 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.
Comment #19
Wim LeersKernel tests containing knowledge of internal implementation details is once again getting in the way.
Comment #20
gabesulliceOne line, easy fix.
Comment #21
gabesulliceWhoops, back to NR
Comment #22
tedbowYep looks good!
Comment #23
larowlanComment #24
larowlanI 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:
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
Comment #25
Wim LeersFixed a deprecation error that went unnoticed because this was rolled against 8.7.
Comment #26
Wim LeersAddressed #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. 😲
Comment #27
Wim Leers🤓 Formatting nits here.
🤓 We don't need most of these entity schemas to be installed because we're not saving such entities.
Fixed.
Comment #28
Wim LeersFYI
entity_test_no_bundle
was specifically chosen because it does not list abundle
entity key! This is what @larowlan was getting at in #24. Took me some time to grok 😇Comment #29
gabesulliceThanks @Wim Leers!
Just fixing two nits...
Nit: Let's not call
->getBundleInfo()
twice.Nit: One has
()
and the other does not.Comment #30
larowlanSomething went wrong with that patch @gabesullice, patch size dropped dramatically and it no-longer applies
Comment #31
gabesulliceGah, of course. Sorry!
Comment #32
Wim LeersComment #34
larowlanCommitted 6d3b05c and pushed to 8.8.x. Thanks!
🎧 this commit was brought to you by henchlock by thee oh sees
Comment #35
gabesulliceAny chance of this getting backported since it's a bug and it's blocking the JSON:API Schema module when the
comment
module is enabled?Comment #36
gabesulliceComment #37
Wim LeersComment #38
larowlanC/p as 8adf90654d and pushed to 8.7.x
Comment #41
larowlanRolled 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
Comment #42
gabesulliceComment #43
Wim Leers#42's fix looks good and it passed PHP 5 testing so 👍
Comment #44
larowlanCommitted e731e88 and pushed to 8.7.x. Thanks!
Comment #46
claudiu.cristeaAdded #3077607: Entity types w/o bundle key should not be able to define multiple bundles as related.
Comment #48
Luke.LeberI 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):
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 :)
Comment #49
gabesulliceHm, @Luke.Leber, it seems really bizarre to me that this issue would have caused the regression you're describing. Do you mind describing how you determined/confirmed that this patch caused the regression and/or a little more detail/explanation about the code that was impacted?
This ^ is the entirety of the patch, excluding tests. It's really difficult for me to see how that would cause this:
Maybe you have some extra insight into how it reverberated through the codebase to cause your issue?
Comment #50
Luke.LeberGood 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
Comment #51
gabesulliceI would also try with the Comment entity, since that was the entity type which this issue intended to fix.
Comment #52
Luke.LeberHello,
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
Comment #53
Wim LeersPer #52.