Properly deprecate EntityFormInterface::setEntityManager() and trigger an error on use

Reroll and adding deprecated to the entity.manager service to see what happens if we do this.

+++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityFormTest.php
@@ -23,11 +23,7 @@ public function testEntityManagerDeprecation() {
     $time = $this->prophesize(TimeInterface::class)->reveal();
     $form = new ContentEntityForm($entity_manager, $entity_type_bundle_info, $time);
-
-    $reflected_form = new \ReflectionClass($form);
-    $entity_manager_property = $reflected_form->getProperty('entityManager');
-    $entity_manager_property->setAccessible(TRUE);
-    $this->assertTrue($entity_manager_property->getValue($form) === $entity_manager);
+    $this->assertTrue($form->entityManager === $entity_manager);
   }

I'm not really sold yet on the whole private thing here. If we do this then we should IMHO just clean it up properly like everything else, drop the property and rely on the trait.

Specifically, this doesn't test anymore what it used to test, the BC layer in the constructor. Setting the private property there doesn't do anything anymore and is not what is tested here, so we can just as well drop it entirely there and adopt a standard construct BC layer. This was the first such constructor change that we did around entity manager.

Thanks for the explanation, makes sense now what it does.

I think it comes down to whether this is something we care about:

The reason the trait doesn't work in this case is because the trait would break the behavior of the setEntityManager() injector. The trait will always get the service from the container, so the injector would no longer do anything. In normal mode, this is probably fine, but if someone injects a mock using that method or is testing something in a unit test, the test would break.

Do we care about such IMHO rare cases that would AFAICS only break (unit) tests which would quite likely already be broken as we ourself on longer use $this->entityManager.

Or are we just going to say that code needs to be updated anyway and adding that logic to support it isn't worth it. IMHO, it's not :)

+++ b/core/lib/Drupal/Core/Entity/EntityForm.php
@@ -429,6 +456,7 @@ public function setModuleHandler(ModuleHandlerInterface $module_handler) {
     $this->entityManager = $entity_manager;

Shouldn't this set the private entity manager now? ie. $this->privateEntityManager? Or we want this to invoke the magic __set()? I guess it doesn't matter. Looks odd though.

Re the trait vs private discussion - I'm assuming that @Berdir is suggesting we use \Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait and @mikelutz is pointing out that this would break the \Drupal\Core\Entity\EntityForm::setEntityManager() method.

At this point I'm not sure it really matters either way. I think on balance I'd err towards @mikelutz's solution because we're adhering better to the BC promise. Therefore going to rtbc.

Looks good. I should have spotted the message formats. Nice work.

Committed 668e08a and pushed to 8.8.x. Thanks!

Unfortunately, this patch breaks any subclasses that are lazy about declaring class properties: #3094913: EntityForm's use of magic setter/getter breaks any subclasses that don't declare a property.

@joachim Same here #3097934: Fatal error on admin page

I'm wondering if @mikelutz for BC it should create an array property for default getters and setters that are used?