Deprecated service entity.manager needs to be replaced with entity_type.manager in ContentEntityForm

the entity type manager and entity manager is already injected in the parent.

The problem is that this would break a ton of subclasses that also use constructor injection

While working on commerce_pos, we discovered at Acro Media that in order to extend the Entity form, we had to use a deprecated object. I humbly recommend we move forward by providing a more future-proof additional form object that we can extend that doesn't require the use of deprecated classes.

Also, I've updated the first 5 issues I found throughout the contrib module space where everyone was dealing with this headache in very similar ways.

Well, deprecating classes will also force everyone to update all their classes, and the class hierarchy that we have in core alone here is huge. And the second problem there is naming, I'm not sure what else we should name it, possibly a different namespace.

We also can't remove the argument without reordering the others, so really not sure.

This is first attempt to resolve this issue.
Please post your views.

I am not able to figure out, what's the issue?
Can anyone help me out?

+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -127,15 +127,471 @@ public function form(array $form, FormStateInterface $form_state) {
+/**
+ * Entity form variant for content entity types.
+ *
+ * @see \Drupal\Core\ContentEntityBase
+ */
+class ContentForm extends EntityForm implements ContentEntityFormInterface {

Putting 2 classes in the same file breaks PSR-0 and would make ContentForm not autoloadable. I also don't understand why there are so many changes to the existing class.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -127,15 +127,471 @@ public function form(array $form, FormStateInterface $form_state) {
-    $form['footer'] = [
-      '#type' => 'container',
-      '#weight' => 99,
+    return $form;
+  }

This is why the test failed. But as said above I'm not sure why there is so much change to ContentEntityForm

I think before we start work on this one we need to work out what's the best way forward given #3 and #6.

So I had this idea...

I guess it's far less likely that any actual subclasses override it, so I guess we can just remove it from those.

And here is a version that removes it from the constructor in all subclasses in core. There are actually fewer than I expected.

Also added an additional service to replace the single remaining usage of $this->entityManager in ContentEntityForm but did not replace it in the subclasses yet, not sure if that should be done here as well or not. Might overlap with other issues.

Forgot to add a @param.

Surely we want to mention this, so tagging now while I think of it. We'll want to note a link to the CR in an @see in the code, right?

Well, this is not a new deprecation, we already have https://www.drupal.org/node/2549139 and we also have dozens of other places where we will need to remove it.

Also, it's not like you need actually *do* anything, it hasn't been replaced with something, you can just remove the argument, that doesn't really need a before/after code snippet to explain? (It works because the parent injects both the entity manager and entity type manager service through setter methods defined on EntityFormInterface.. unfortunately this was added after the constructor injection in this class)

+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -43,21 +35,35 @@ class ContentEntityForm extends EntityForm implements ContentEntityFormInterface
+  public function __construct($entity_type_bundle_info = NULL, $time = NULL, $entity_repository = NULL) {
+    if ($entity_type_bundle_info instanceof EntityManagerInterface) {
+      @trigger_error('Passing the entity.manager service to ContentEntityForm::__construct() is deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0.', E_USER_DEPRECATED);
+      $this->entityTypeBundleInfo = \Drupal::service('entity_type.bundle.info');
+      $this->time = \Drupal::service('datetime.time');
+      $this->entityRepository = \Drupal::service('entity.repository');
+    }
+    else {
+      $this->entityTypeBundleInfo = $entity_type_bundle_info ?: \Drupal::service('entity_type.bundle.info');
+      $this->time = $time ?: \Drupal::service('datetime.time');
+      $this->entityRepository = $entity_repository ?: \Drupal::service('entity.repository');
+    }

So I think there is another approach to this that gives us type-safety. We could remove all the arguments and use func_get_args() and then depending on the number of args we can call either legacyConstructor or constructor (no __) and those methods can have type hinting. The constructor might need bike-shedding.

I think better to file CR about this removal, I bet lots of contrib will be affected, ++ to rtbc

> So I think there is another approach to this that gives us type-safety. We could remove all the arguments and use func_get_args() and then depending on the number of args we can call either legacyConstructor or constructor (no __) and those methods can have type hinting. The constructor might need bike-shedding.

Thought about that a bit and I'm not so sure that it is clearer.

* I assume we would still keep the @param documention that we would expect, so at least looking at that will show you what kind of arguments you should pass it. I'm however pretty sure that our phpcs configuration will complain about that and if not yet, then it will do so at some point. (it might also complain about missing type hints eventually..
* A func_get_args() and then a call_user_func_array() would be unecessary overhead for every call, even those that are passing in the correct parameters. While there are actually fewer subclasses of this in core than I expected (https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...), I know there are lots and lots of them in contrib and custom and a majority doesn't bother with dependency injection. For example because drupal console generates empty classes as a starting point and tons of contrib/custom entity types start with that and never delete it :)
* Not having the arguments makes it harder to use tools like phpstorm to override the method (which copies it and calls the parent as a starting point). You'd still need to add the type hints now, but otherwise you have to manually add all the arguments too.
* We also end up with actual methods that we would I guess need to declare as @internal as we probably want to remove them again in 9.0 and rename the right one back to _construct(), risking breaking someones code that called them directly. So more risks and work there.

If this is about ensuring that the arguments we *do* get are of the correct type, then IMHO we can handle that with explicit assert() or conditions that fail if there is an unexpected value, giving us basically the same guarantee with fewer side effects and overhead. I'm not sure what other reasons you have for suggesting your approach?

One thing that I was not sure about is if this is OK for the stricter inheritance checks in PHP 7.2, but according to https://3v4l.org/AJEEX it is and I also triggered a test on PHP 7.2 now to be sure.

@Berdir thanks for thinking that through. Good points.

So looking at your approach. I think we can do it cleaner if we only remove the typehint from the first argument and just check that one.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -43,19 +36,36 @@ class ContentEntityForm extends EntityForm implements ContentEntityFormInterface
+      $this->entityRepository = \Drupal::service('entity.repository');
...
+      $this->entityRepository = $entity_repository;

Another thought it we could just do $this->entityRepository = $entity_repository; because the entity manager implements EntityRepositoryInterface. Ah ha... I've just realised this means we can actually still type hint on the method because EntityManagerInterface extends EntityRepositoryInterface so all current implementations will pass in something that satisfies EntityRepositoryInterface.

Ha, of course, makes sense, I only added that argument later when I noticed we need that for that remaining call, didn't re-think it then.

Looks ready like that to me. Still not sure on the whole CR thing, we also added new optional arguments before to the very same class and didn't do change records for those I think. And figuring out what this to be done here is still trivial if you get the deprecation message. IMHO, we don't need another change record here, we're just adopting to an already existing one.

What we do need to do is update the deprecation message, because now you need to replace it with the entity.repository service instead. Needs work for that. And possibly the other arguments should have deprecation messages too, although that is probably a different issue..

+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -43,19 +36,28 @@ class ContentEntityForm extends EntityForm implements ContentEntityFormInterface
+      @trigger_error('Passing the entity.manager service to ContentEntityForm::__construct() is deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0.', E_USER_DEPRECATED);

this message require link to CR, probably https://www.drupal.org/node/2549139 just needs update

Fixed the noticed and added a test for it. I think updating https://www.drupal.org/node/2549139 on commit is a good idea.

Looks RTBC to me. Updated the issue summary. Did too much of the patch myself to RTBC it, someone else wants to press the button?

Says it needs a CR and IS update, but RTBCing anyway for now since the patch is solid.

This is an existing published CR, we can only update post-commit. And just forgot to remove the IS update tag.

Updating review credits

Committed as 312b93c and pushed to 8.6.x.

Can we get those change record updates now?

Setting status to 'Needs work' for those update and title too.

Please set to fixed when done and reset the title when done.

Still not quite sure why this change requires a change record update when we already did tons of backwards-compatible constructor changes that we did not document as change record, even on the very same class :)

Anyway, added an Additional changes chapter to https://www.drupal.org/node/2549139 and mentioned it there.

Also a follow-up: #2969109: Replace deprecated usages of entityManager in form classes

I don't think the BC layer here is complete. This currently breaks webform in 8.6 because we pass entityManager to the parent and then expect it to be available as a property.

I believe we should continue to assign the service to entityManager to maintain BC.

This should do the trick.

AFAIK webform was already updated in dev.

The entity manager will be available in later methods, always, due to \Drupal\Core\Entity\EntityForm::setEntityManager(), it's just in the constructor where it doesn't work like that.

That said, leaving it to @alexpott to decide if we want to do this, don't really care :)

Ah, indeed! The version I have installed actually does the following:

  public function __construct(...) {
    parent::__construct($entity_manager);
    ...
    $this->storage = $this->entityManager->getStorage('webform_submission');

So the examples a linked in #35 aren't actual examples of this breaking, so updating webform is also a viable fix. Given the minimal impact/disruption of #35, it might be worth it anyway. Agree with #36, @alexpott can decide.

@catch pointed me to this issue after finding that we may have introduce the same in #2981025-16: Replace injection of deprecated entity.manager service with entity_type.manager in \Drupal\Core\Field\Plugin\Field\FieldFormatter\StringFormatter and LanguageFormatter

Committed/pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!

Hm, doesn't look like the push went through?

You're right - sorted now though.