Support entities that are neither content nor config entities

Woah! 🤯 This would actually be the first tine I *ever* hear about an entity type that is neither content nor config. Pretty much all code in Drupal core assumes it’s either of the two.

To my knowledge, Drupa core’s REST API also assumes this.

Any chance you could share this custom entity type? If not, can you tell us why it’s neither content nor config? 🙏

You’ve piqued my curiosity!

Evidently I agree with you that this should not break. Understanding the rationale behind this would help determine how we approach solving this.

@Wim

Check the Apigee Edge module, we have managed to build entities that are not content entities but still fieldable and support 80% of what content entities can do/have in Drupal 8. We did not need revisioning yet, this is what the remaining 20% would cover. Moreover, in our implementation the entities are API responses turned into full-fledged Drupal entities, so they are not even coming from and being saved to a PDO compliant database.

While we were working on this we bumped into lots of problems, because as you mentioned, Drupal core and majority of the Drupal 8 modules assumes that every entity is a content- org a config entity. Although, they should rely on interfaces, like FieldableEntityInterface or RevisionableInterface.

https://github.com/apigee/apigee-edge-drupal/blob/8.x-1.x/src/Entity/Fie...
https://github.com/apigee/apigee-edge-drupal/blob/8.x-1.x/src/Entity/Att...

Ping me on Drupal slack if you would like to know more.

Patch with an initial fix.

Okay, so this requires a bigger change in the module as I initially expected.

\o/ Should I restore the assert for the entity type (class) in \Drupal\jsonapi\ResourceType\ResourceTypeRepository::getFieldMapping() or it is not necessary?

Although, they should rely on interfaces, like FieldableEntityInterface or RevisionableInterface.

They should, but they can't. You can't do if ($x instanceof RevisionableInterface) on content entities, because every content enitty implements this interface (interface ContentEntityInterface extends \Traversable, FieldableEntityInterface, TranslatableRevisionableInterface, SynchronizableInterface): you need to check for each given entity whether their entity type's \Drupal\Core\Entity\EntityType::isRevisionable() returns TRUE…

Yes, this is very unfortunate, and trips up everyone all the time (me included).

#3: You linked to https://github.com/apigee/apigee-edge-drupal/blob/8.x-1.x/src/Entity/Fie..., but that's also hosted on Drupal.org's git repos: https://cgit.drupalcode.org/apigee_edge/tree/src/Entity/FieldableEdgeEnt... … and AFAICT because it contains Copyright 2018 Google Inc. and a GPL2-only clause, it is violating https://www.drupal.org/about/licensing in two ways:

1. There can't be copyright attributed to a person or organization, it must be GPL2+.
2. GPL2-only is not the same as GPL2+: https://www.drupal.org/about/licensing#gpl2-only-gpl3-only

Licensing issues aside, https://www.drupal.org/project/apigee_edge is a really fascinating Drupal module! It seems to be kind of like https://www.drupal.org/project/external_entities: to model external data and interact with it (even write to it) as if they were local Drupal entities. But much more deeply integrated. Really, really interesting!

I'd love to read more about why you chose to architect it this way. What benefits are you getting from doing all the work to model this external data as Drupal entities? In any case: 👏👏👏

The patch in #9 looks great. But JSON:API was recently moved into Drupal core, so this will need to wait until we can make commits in the Drupal core 8.7.x branch, to ensure the contrib module for Drupal 8.6 and the core module for Drupal >=8.7 stay in sync.

Remarks:

1. It'll still need to update many docs in the touched files — but totally makes sense to wait for a general +1 prior to doing that 🙂.
2. It should be rolled with -M5% to make it more clear that class FieldableEntityDenormalizer is mostly existing code.

Really curious to read what my fellow maintainers @e0ipso and @gabesullice think about this: both about the desire to support entities other than content/config, and the Apigee Edge Drupal module.

#12 Thanks Wim for your input. We are going to work on this licensing issue in the next two weeks.

#13 Also thanks for the kind words. I was always "obsessed" with the idea of onboarding remote entities to Drupal as full-flagged Drupal entities and I saw plenty other ppl try to solve the same issue. I probably bumped into external entities module before I started to implement our own solution. (The API that we integrated to Drupal sometimes behaves in a very specific way so any generalized solution would not work for us.)
I always planned to write a glossary about this module and the approach that we managed to solve this problem. Because we were in a rush to release the first stable of the module I did not have time. Now that the module is close to the first stable release I may have some time to make up this leeway.

#14 Yes, I followed the news :) Congratulations by the way. ;) Let me know when it gets clear how we can proceed with this PR further.

I always planned to write a glossary about this module and the approach that we managed to solve this problem. Because we were in a rush to release the first stable of the module I did not have time. Now that the module is close to the first stable release I may have some time to make up this leeway.

Really looking forward to that! I'd suggest also publishing it as a blog post. I think it may inspire other people to do something similar :) Or at least to look at the problems they need to solve differently, thanks to knowing this is actually feasible. I'd be happy to provide feedback even during the writing process (e.g. a Google Doc where you grant me only "comment" permission).

Sorry for the silence here — I attended two conferences in the first two weeks of April.

1. Thanks for working towards a patch that passes tests in https://www.drupal.org/project/jsonapi, but since JSON:API got committed to core, we'll need to land this in Drupal core first.

2. +++ b/src/Controller/EntityResource.php
   @@ -1049,7 +1048,7 @@ class EntityResource {
      protected function updateEntityField(ResourceType $resource_type, EntityInterface $origin, EntityInterface $destination, $field_name) {
        // The update is different for configuration entities and content entities.
   -    if ($origin instanceof ContentEntityInterface && $destination instanceof ContentEntityInterface) {
   +    if ($origin instanceof FieldableEntityInterface && $destination instanceof FieldableEntityInterface) {
          // First scenario: both are content entities.
          $field_name = $resource_type->getInternalName($field_name);
   
   +++ b/src/JsonApiResource/ResourceObject.php
   @@ -211,10 +212,14 @@ class ResourceObject implements CacheableDependencyInterface, ResourceIdentifier
      protected static function extractFieldsFromEntity(ResourceType $resource_type, EntityInterface $entity) {
   

   The comments here need to be updated.

3. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -263,14 +262,15 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +    $rc = new \ReflectionClass($entity_type->getClass());
   

   Why do we need reflection?

4. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -281,9 +281,8 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   -    else {
   -      throw new \LogicException("Only content and config entity types are supported.");
   -    }
   +
   +    return [];
   

   I don't think this change is necessary given that the first if-test is now checking for FieldableEntityInterface instead of ContentEntityTypeInterface?

So now JSON API is in core, I'll create a new patch just for the Drupal 8.7.x branch or this could be fixed only in 8.8.x?. Do we still need a patch that fixes this issue in the contrib module that users Drupal 8.6.x are still using?

1. Rerolled the patch. Created an interdiff between #9 and #21 but I had to manipulate paths in #9 because it was created when the module was only a contrib.

2. Updated.

3. Not sure what is the problem with reflection in this situation but replaced it with is_a().

4. An exception should not be thrown here, it could happen that an entity is neither fieldable nor a config entity. For example, it is only revisionable.

Not sure what is the problem with reflection in this situation but replaced it with is_a()

Reflections are a heavier inspection API, compared to instanceof of an is_a() alternative.

Forgot to attach the interdiff file.

1. +++ b/core/modules/jsonapi/src/JsonApiResource/ResourceObject.php
   @@ -267,12 +272,28 @@ protected static function buildLinksFromEntity(ResourceType $resource_type, Enti
   +    @trigger_error('\Drupal\jsonapi\JsonApiResource\ResourceObject::extractContentEntityFields() has been deprecated in favor of \Drupal\jsonapi\JsonApiResource\ResourceObject::extractFieldableEntityFields(). Use that instead.');
   

   Do we want to also add a @deprecated tag explaining that as well?

2. +++ b/core/modules/jsonapi/src/Normalizer/ContentEntityDenormalizer.php
   @@ -2,6 +2,8 @@
   +@trigger_error('\Drupal\jsonapi\Normalizer\ContentEntityDenormalizer has been deprecated in favor of \Drupal\jsonapi\Normalizer\FieldableEntityDenormalizer. Use that instead.');
   +
   

   This will trigger without really using the normalizer.

   Let's add the deprecation in the service definition instead.

   https://symfony.com/doc/current/service_container/alias_private.html#dep...

Additionally since we copied the denormalizer code to the new class we should have the deprecated class be a clone of the new one:

final class ContentEntityDenormalizer extends FieldableEntityDenormalizer {}

+++ b/core/modules/jsonapi/jsonapi.services.yml
@@ -51,6 +51,11 @@ services:
+  serializer.normalizer.fieldable_entity.jsonapi:
+    class: Drupal\jsonapi\Normalizer\FieldableEntityDenormalizer
+    arguments: ['@entity_type.manager', '@entity_field.manager', '@plugin.manager.field.field_type']
+    tags:
+      - { name: jsonapi_normalizer }

The old service should be deprecated and we should have an alias to the new one.

https://symfony.com/doc/current/service_container/alias_private.html#ali...

#20:

Do we still need a patch that fixes this issue in the contrib module that users Drupal 8.6.x are still using?

All issues that land in core are backported to the contrib module by the maintainers. Landing it in core should be enough.

1. Added

2.

1. deprecated added to the service definition

2. Additionally since we copied the denormalizer code to the new class we should have the deprecated class be a clone of the new one

   FieldableEntityDenormalizer is final as it should so this is not possible.

3. Service alias added.

Just one more thing:

1. +++ b/core/modules/jsonapi/src/JsonApiResource/ResourceObject.php
   @@ -267,12 +272,32 @@ protected static function buildLinksFromEntity(ResourceType $resource_type, Enti
   +   * @deprecated in Drupal 8.7.x and will be removed before Drupal 9.0.0.
   

   We can't deprecate things in 8.7 anymore since that already shipped. We can deprecate things in 8.8.

Comments addressed in #29 & added an interdiff.

Thanks, @yogeshmpawar!

I think we should at some dedicated tests here.

I agree with @jibran - we should be adding some testing coverage so we don't break this again in the future.

I'll try to add something similar that I added in https://www.drupal.org/project/drupal/issues/2942529#comment-13099844.

Great, thanks @mxr576!

This should be 8.8.x-dev now right?

Correct, thanks!

Sorry, I do not have a capacity now to work on the test for this patch. I checked it and this module's test suite is more robust and complicated than the Editor's and I am not sure where should I add these additional test cases.

Also, we kinda stuck with @alexpott how we should test the Editor module procedural functions in the other issue.

#31 patch working fine. Thanks to @yogeshmpawar, @mxr576, @Wim Leers for the support

Per #34, this issue just needs tests.

When installing jsonapi_extras with D9 and patch #31, I got the following error:

Uncaught PHP Exception Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: "The deprecation template must contain the "%alias_id%" placeholder." at [drupalroot]vendor/symfony/dependency-injection/Alias.php line 107

Rerolled patch to fix that error and created a corresponding merge request at https://git.drupalcode.org/project/drupal/-/merge_requests/71

Patch #46 has an error. Hopefully this one works better.

I realized I was trying to apply these patches using composer without the patchLevel -p2 setting. Attaching new patch with interdiff.

Seems this one is pretty close to being done, mostly needed some tests. I think it would be helpfull if we have a bit of a plan on which tests we want. This way the implementation of these test could be moved along by other contributors.

Woah! 🤯 This would actually be the first tine I *ever* hear about an entity type that is neither content nor config. Pretty much all code in Drupal core assumes it’s either of the two.

I'm planning to include the first non Content non Config entity to core as part of #2401463: Make dblog entities. I'm glad to see this issue is already in progress.

#Patch 50 is working fine.

Any updates on this issue for 9.3.6 or 9.3.7. Getting Support entities that are neither content nor config entities after trying patch https://www.drupal.org/files/issues/2020-12-02/3042467-50.patch. Using Google Apigee Edge modules and teams. Sorry for the double post, fat fingers.

Any updates on this issue for 9.3.6 or 9.3.7. Getting Support entities that are neither content nor config entities after trying patch https://www.drupal.org/files/issues/2020-12-02/3042467-50.patch.

Experience an issue when upgrading drupal with this patch applied and the latest version of jsonapi_extras module.

I have created a patch against jsonapi_extra's that worksaround the issue.

I do not fully understand why my workaround works and suspect it's the patch on this issue that should be updated, not jsonapi_extras.

https://www.drupal.org/project/jsonapi_extras/issues/3308517

I had the same issue for 9.5.2 drupal and brightcove module. #Patch 50 fixed the issue, thanks!

Thanks @zakiya !

I had facing the same issue for 9.5.1 Drupal and JSON:API module. #Patch 50 is fixed my issue.

My Error is "LogicException: Only content and config entity types are supported. in Drupal\jsonapi\ResourceType\ResourceTypeRepository->getAllFieldNames() (line 340 of /var/www/html/devportal/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php)."

Changes that I have made:

* Created an initial CR draft for this change: #3343351
* Rerolled #50 because it did not apply on Drupal core 10.1.x
* Correct deprecation messages in #50

@thursday_bw I have also experienced the issue that you have mentioned in #59 but only on Drupal 9.4.x. This is a pretty weird one. I did some debugging and I believe this line causes that problem because if I change (correct?) that line to $original = $container->getDefinition($service_id); then the issue disappears. Also, the good new is that this code was removed and deprecated and removed since Drupal 9.5.0, see https://github.com/drupal/core/blob/10.0.3/lib/Drupal/Core/DependencyInj... and #2531564: Fix leaky and brittle container serialization solution.

I am not sure why I did not expect changes in JSONAPI around content entity normalization since 2019...

9.4.x failures are just deprecations, so they are fine.

  128x: The "serializer.normalizer.content_entity.jsonapi" service is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. You should use the 'serializer.normalizer.fieldable_entity.jsonapi' service instead.
    128x in HelpTopicsSyntaxTest::testHelpTopics from Drupal\Tests\help_topics\Functional

So it turned out that what I was suggested to @thursday_bw in #63 was not a solution, just silenced the problem. With that fix serializer.normalizer.content_entity.jsonapi was not registered therefore field enhancers provided by JSONAPI Extras did not work when an input needed to be transformed. (They worked when the output needed to be un-transformed.)

Something weird is going on with the service container in Drupal 9.4 (and most probably 9.5 and above), or just the way how JSONAPI registers and collects normalizers conflicts with the service deprecation with service alias approach. After changed my code like below, everything started to work again and DependencySerializationTraitPass did not what to be tweaked.

   serializer.normalizer.content_entity.jsonapi:
-    alias: serializer.normalizer.fieldable_entity.jsonapi
-    deprecated: The "%alias_id%" service is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. You should use the 'serializer.normalizer.fieldable_entity.jsonapi' service instead.
+    class: Drupal\jsonapi\Normalizer\FieldableEntityDenormalizer
+    arguments: ['@entity_type.manager', '@entity_field.manager', '@plugin.manager.field.field_type']
+    deprecated: The "%service_id%" service is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. You should use the 'serializer.normalizer.fieldable_entity.jsonapi' service instead.
+    tags:
+      - { name: jsonapi_normalizer }

:fingers-crossed:

Okay, I have no idea currently how to deprecate a normalizer without using service aliases and not triggering deprecation errors...
But that just blocking the merge of this change, just as test coverage, which requires more work than I have expected because fundamentals are still missing in core for building entities that are not content entities or config. (I guess a "skeleton" fieldable entity implementation cannot be accepted, it must be functioning... does it? :thinking-face:)

Back to using an MR... I am tired of generating interdiffs :)

I hope this is a good enough approach to consider that this change is covered by tests without spending more time to get this fix merged.

This should pass with deprecations, which is expected, I haven't silenced them.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

Thanks @needs-review-queue-bot, fixed it!

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Surprised the bot hasn't caught this but the MR no longer applies.

What happened again.... :-(

https://github.com/drupal/core/commit/4f183ba36780d1ddbdeca109650ef14ded...
https://github.com/drupal/core/commit/68ef3c4832eb8f257a2eeef1f85a4fe0ba...

Left a review, it needs change message to 10.2 and add deprecation tests

Created a new 10.3.x backport patch from the most recent changes.

Uploading a new 10.3.x backport, because 10.3.4 introduced changes in `core/.deprecation-ignore.txt` (https://git.drupalcode.org/project/drupal/-/commit/83a80f66982d40ea7cb83...) and the patch doesn't apply anymore. I removed all test related changes from the patch, so those won't cause trouble in compatibility.

Backported to 11.3.x. The patch does not contain the tests.

MR#15540 should work for Apigee.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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