Remove BC layers from the entity system

Expanding the scope here to the whole Entity component.

Some fixes.

Fixed a pretty stupid mistake and removing some legacy tests. Not quite sure what to make of SqlContentEntityStorageSchemaConverter.php, there are some tests failing that use it in entity_test, I guess that will be removed in the update path removal issue.

Should mostly be just update path test failing now. So this is as far as we'll get until those are gone.

Reroll and combined with the update removal patch to see if there's something left after it.

Actual patch is quite a bit smaller as some things were already removed.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
@@ -24,7 +24,7 @@ class ContentEntityType extends EntityType implements ContentEntityTypeInterface
-  protected $requiredRevisionMetadataKeys = [];
+  protected $requiredRevisionMetadataKeys = ['revision_default' => 'revision_default'];

We introduced the property just because to allow extending the revision metadata keys when the BC layer is used, but now that it is being removed I think we should remove the property as well and instead add the default revision metadata keys in ContentEntityType::__construct() like we do for the entity keys in EntityType::__construct().

Note that the BC layer for revision metadata keys is being removed in #3099789: Remove the BC layer for revision metadata keys, so those parts should be taken out from this patch at some point.

Yeah, since that is non-trivial, I think I'll just wait until that is committed and then reroll this. We have quite a bit of a dependency chain right now ;)

The update path is gone, so here's a reroll with some additional removals.

Still postponed on the metadata issue, will reroll and resolve the conflicts with that when it is in.

This is postponed on #3099789: Remove the BC layer for revision metadata keys, and which other issue?

The one that #3099789: Remove the BC layer for revision metadata keys was blocked on but looks like the new plan is to do them in parallel, one for 8.x only and that for 9.x.

Rebased.

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -443,13 +431,6 @@ public function entityClassImplements($interface) {
-  public function isSubclassOf($class) {
-    return $this->entityClassImplements($class);
-  }

This was never deprecated with trigger_error() and seems used quite heavily in contrib: http://grep.xnddx.ru/search?text=isSubclassOf&filename=

As it doesn't really hurt to leave it we could bump removal of this to 10.x?

Woot, this has 23 @deprecated which is 59% of all the remaining ones (where some of the remaining ones are for testing deprecation tooling itself). So this would singlehandedly remove almost everything left deprecated in Drupal 9.

I agree with @longwave - let's properly deprecate isSubclassOf() and move it to D10. We can do that in another issue though - so let's not make a change to it here.

5 pages on http://grep.xnddx.ru/search?text=isSubclassOf&filename= and maintaining the method with no logic is not that bad.

Opened #3115608: Properly deprecate EntityType::isSubclassOf()

Although @init90 informed me that the issue already exists: #3113020: Properly deprecate \Drupal\Core\Entity\EntityTypeInterface::isSubclassOf()

Back to needs work for removing the isSubclassOf() removal.

Added that back.

Fixed CS and useless var

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1244,16 +1244,9 @@ public function __clone() {
   +    if (($label_key = $this->getEntityType()->getKey('label'))) {
   +      return $this->getEntityKey('label');
   

   label_key is unused now

2. +++ b/core/lib/Drupal/Core/Entity/EntityBase.php
   @@ -148,24 +148,9 @@ public function bundle() {
   +    if (($label_key = $this->getEntityType()->getKey('label')) && isset($this->{$label_key})) {
   +      return $this->{$label_key};
   

   is bit different from previous

Still a number of references to label_callback which is removed here. I haven't looked into it but why doesn't EntityTestLabelCallback fail now, given its name?

core/tests/Drupal/KernelTests/Core/Entity/FieldSqlStorageTest.php
277:    $entity_type = $bundle = 'entity_test_label_callback';
278:    $this->installEntitySchema('entity_test_label_callback');

core/modules/rest/src/Entity/RestResourceConfig.php
25: *   label_callback = "getLabelFromPlugin",

core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
102:   * a 'label_callback', but not a 'label' entity key. For example: User.

core/modules/system/tests/modules/entity_test/src/Entity/EntityTestLabelCallback.php
9: *   id = "entity_test_label_callback",
12: *   base_table = "entity_test_label_callback",
13: *   label_callback = "entity_test_label_callback",

If the 'handler' key is removed from SelectionPluginBase it feels like this code in SelectionPluginManager::getInstance() can be refactored away now? We shouldn't need $options['handler'] at all any more?

    // Initialize default options.
    $options += [
      'handler' => $this->getPluginId($options['target_type'], 'default'),
    ];

    // A specific selection plugin ID was already specified.
    if (strpos($options['handler'], ':') !== FALSE) {
      $plugin_id = $options['handler'];
    }
    // Only a selection group name was specified.
    else {
      $plugin_id = $this->getPluginId($options['target_type'], $options['handler']);
    }
    unset($options['handler']);

Thanks for the review.

#27: entity_test_label_callback still had a label callback and was used in one other test but that just wants a long entity type id that's long, so I gave it an even longer one. Deleted the entity type as it was just in a test module.

Updated the docblock for the rest test class and removed the bogus definition there (label callbacks can't be methods, that was never called)

#28: This is still used IMHO, see for example \Drupal\Core\Entity\EntityAutocompleteMatcher::getMatches(), we no longer have it in the settings but it's still passed in there to getInstance() as that's the only argument that we have.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -374,31 +374,6 @@ public function calculateDependencies() {
-  public function urlInfo($rel = 'edit-form', array $options = []) {
...
-  public function url($rel = 'edit-form', $options = []) {
...
-  public function link($text = NULL, $rel = 'edit-form', array $options = []) {

🔥This helps a lot with #2491981: There are too many ways to generate URLs and links 🥳

I retract #28, it is only the default that has been removed and we still need 'handler' otherwise.

+++ b/core/modules/rest/src/Entity/RestResourceConfig.php
@@ -22,7 +22,6 @@
- *   label_callback = "getLabelFromPlugin",

There is a getLabelFromPlugin() method that can also be removed.

Fix #31

Nothing left to do here, let's get this done!

Some random thoughts:

• The good news is there's not a lot of contrib affected by the label_callback deprecation - http://grep.xnddx.ru/search?text=label_callback&filename=
• There's going to be plenty of modules that have to clean up Drupal\Core\Entity\Entity usage - but that was deprecated properly and any modules that we typehinting on it were already broken.
• The deprecation of 'type' => 'hidden' is going to painful - http://grep.xnddx.ru/search?text=%27type%27%20%3D%3E%20%27hidden%27&file... - as a programmatic deprecation tools like PHPStan can't detect this.

Not sure what to do about the last one. I think that that is going to catch some people out but at least once they've fixed their code it will still work on D8.

One thing we definitely have to do is:

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -1005,13 +1005,6 @@ public function toUrl($rel = 'edit-form', array $options = []) {

@@ -1005,13 +1005,6 @@ public function toUrl($rel = 'edit-form', array $options = []) {
     return $this->storage->toUrl($rel, $options);
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  public function link($text = NULL, $rel = 'edit-form', array $options = []) {
-    return $this->storage->link($text, $rel, $options);
-  }
-
   /**
    * {@inheritdoc}
    */
@@ -1169,13 +1162,6 @@ public function referencedEntities() {

@@ -1169,13 +1162,6 @@ public function referencedEntities() {
     return $this->storage->referencedEntities();
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  public function url($rel = 'edit-form', $options = []) {
-    return $this->storage->url($rel, $options);
-  }
-
   /**
    * {@inheritdoc}

We're missing the removal of \Drupal\views_ui\ViewUI::urlInfo()

Here is the removal of Drupal\views_ui\ViewUI::urlInfo().

1) Drupal\BuildTests\Framework\Tests\BuildTestTest::testPortMany
RuntimeException: Unable to start the web server.
ERROR OUTPUT:

Random fail? Retest requested and setting back to RTBC.

> There's going to be plenty of modules that have to clean up Drupal\Core\Entity\Entity usage - but that was deprecated properly and any modules that we typehinting on it were already broken.

Sure about that? While I wasn't a fan of the rename, I wouldn't expect a lot of direct usages of this. Pretty much everything is either a config or a content entity or uses EntityInterface as type hint/reference. http://grep.xnddx.ru/search?text=use%20Drupal%5CCore%5CEntity%5CEntity%3B does show 5-ish pages of results, which I guess is more than I expected but managable, considering how widely the entity API is used.

type hidden is a bit unfortunate, but looks like most usages are either hidden base field definitions (where the whole thing is bogus, if you want it hidden, just don't do anything, you can still make it optionally configurable) as well as a considerable amount of old feature exports, which aren't going to hurt. The most unfortunate thing is IMHO actually that nothing really came from it, field_layout is about to be removed from core :)

Committed b57afe5 and pushed to 9.0.x. Thanks!

+++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -88,18 +88,13 @@
    * @param \Drupal\Core\Cache\MemoryCache\MemoryCacheInterface|null $memory_cache
    *   The memory cache.
    */
-  public function __construct(EntityTypeInterface $entity_type, MemoryCacheInterface $memory_cache = NULL) {
+  public function __construct(EntityTypeInterface $entity_type, MemoryCacheInterface $memory_cache) {

We need a follow up issue for:

1. Making the same change in subclasses of this, namely ConfigEntityStorage.
2. Removing the |null from the @param declaration. Both here and in subclasses.

Follow-up has been opened: #3118998: Remove null default for $memory_cache in entity storage handler constructors. Restoring the old status.

This is a pretty massive cleanup; are there any riskier/internally disruptive changes that might merit a release notes?

The trickiest part is possibly #3099789: Remove the BC layer for revision metadata keys, if you'd still rely on the BC layer for your entity type and then update to D9 then... weird things might happen. If you actually use the trait you should get an exception, but it's likely that you don't.

There are some config related removals, those are always tricky, and things might show up incorrect when importing default config in D9, and the label callback would also more or less silently stop working for an entity type, but I think that wasn't too widely used.