Convert Entity to EntityBase for consistency with ContentEntityBase and ConfigEntityBase

Yes.

However, by our coding standard, we'd have to rename it EntityBase.

Right. Also, half of our entity types inherit from ConfigEntityBase anyway, which is properly abstract.

It is abstract now, but hasn't been renamed. No idea if we still want to do that?

I think that it still makes sense doing this as we have ContentEntityBase and ConfigEntityBase both extending from the abstract class Entity. I've done this in the attached patch.

That should be it.

We should probably add a change notice as well?

The IS doesn't seem related to the issue title.

+1 on the change record.

I've updated the issue summary and created a draft change record - https://www.drupal.org/node/3021808

Looks done, the tests all pass and we have a cr that explains why and how. Great work!

As per https://www.drupal.org/core/deprecation#how this needs a @trigger_error to fire when a class extends Entity instead of EntityBase

I've added the @trigger_error. No other changes beside that. There is no diff, as I had to re-roll the patch.

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -2,668 +2,12 @@
+ * \Drupal\Core\Entity\EntityBase instead.

I think this should be intended two spaces.

Also if the patch were rolled with -C I think it would be quite a bit smaller.

Also still needs a test for the deprecation per #22.

I wasn't sure about how to prepare a test and what I've tried didn't worked, because if we create a new class that extends from the deprecated class Entity then the deprecation will be triggered by the autoloader before the test itself.

I've talked with @tstoeckler and he found a solution by using an anonymous class. However anonymous classes are supported only in PHP 7. As we still support PHP 5.6. we both agreed that a test case isn't necessary in this case.

P.S: I wasn't able to find any similar test in core as well, so it looks like we don't offer tests for deprecated classes.

I've prepared the new patch with the -C option. Thank you, @tstoeckler for the suggestion!

Thanks, this looks good to me!

As is mentioned in #25 I really don't see a way for us to test this using our standard mechanisms.

it looks like we don't offer tests for deprecated classes

I think that is supposed to say "tests for deprecated abstract classes", as that is what is causing the problem here. Most of the deprecated abstract classes are tests themselves, for which we generally don't add tests, and I couldn't find any deprecation test for any other abstract class either. For example \Drupal\migrate\Plugin\migrate\process\DedupeBase, doesn't have a corresponding deprecation test.

it looks like we don't offer tests for deprecated classes

I think that is supposed to say "tests for deprecated abstract classes", as that is what is causing the problem here.

Yes, this is what I've meant. Thank you for correcting me and explaining it further! :).

diff --git a/core/lib/Drupal/Core/Entity/Entity.php b/core/lib/Drupal/Core/Entity/Entity.php
index 3ffa411f9c..610b239c8e 100644
--- a/core/lib/Drupal/Core/Entity/Entity.php
+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -2,12 +2,14 @@
 
 namespace Drupal\Core\Entity;
 
-@trigger_error('The ' . __NAMESPACE__ . '\Entity is deprecated in Drupal 8.7.x and will be removed before Drupal 9.0.x. Instead, use ' . __NAMESPACE__ . '\EntityBase. See https://www.drupal.org/node/3021808.', E_USER_DEPRECATED);
+@trigger_error('The ' . __NAMESPACE__ . '\Entity is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Instead, use ' . __NAMESPACE__ . '\EntityBase. See https://www.drupal.org/node/3021808', E_USER_DEPRECATED);
 
 /**
  * Defines a base entity class.
  *
- * @deprecated in Drupal 8.7.x and will be removed in Drupal 9.0.x. Use
+ * @deprecated in Drupal 8.7.0 and will be removed in Drupal 9.0.0. Use
  *   \Drupal\Core\Entity\EntityBase instead.
+ *
+ * @see https://www.drupal.org/node/3021808
  */
 abstract class Entity extends EntityBase {}

Deprecation messages should refer to git tags and real releases not branches. And we need to add an @see to the change record. Fixed on commit.

Committed 47dab9a and pushed to 8.7.x. Thanks!

publish the change record