Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Ever since CommentTypeDeleteForm was first created by #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form, the entity.manager service has been injected into this form. However, it has never been used. In addition to being unused, the entity.manager service is deprecated and should no longer be used.
Proposed resolution
The entity.manager service should not be injected into CommentTypeDeleteForm.
User interface changes
None
API changes
None. Although the protected $entityManager is removed as an instance variable and as a constructor argument, these do not constitute API changes.
Data model changes
None
Original bug report:
Problem/Motivation
CommentTypeDeleteForm class constructor use EntityManager instead of EntityManagerInterface, adding faulty dependency to EntityManager.
Proposed resolution
CommentTypeDeleteForm class constructor should be changed to use the EntityManagerInterface
User interface changes
none
API changes
none
Data model changes
none
| Issue category | Bug, because other implementations of EntityManager won't be possible |
|---|---|
| Issue priority | Normal, because the impact is not that high |
| Disruption | No disruption |
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 2509652-13.patch | 1.86 KB | TR |
| #1 | faulty_dependency_to-2509652-1.patch | 1.73 KB | willzyx |
Comments
Comment #1
willzyx CreditAttribution: willzyx commentedComment #2
willzyx CreditAttribution: willzyx commentedtagging, so it is much easier to find these issues
Comment #3
willzyx CreditAttribution: willzyx commentedAdded beta evaluation
Comment #6
Mile23This bug still exists. Nice catch.
It's also true that
EntityManager(and it's interface) are deprecated.So therefore any work should also fix the deprecation. Since that would change the function signatures, we have to move it to 8.3.x.
Comment #7
willzyx CreditAttribution: willzyx commented@Mile23 the
entity.managerservice is unused inCommentTypeDeleteForm. Since we are talking about changing the constructor signatures might be worth to totally remove the service usage from the class.Comment #13
TR CreditAttribution: TR commentedI have re-written the issue summary.
The original issue is largely moot because the entity.manager service is never used, so instead of changing the type from EntityManager to EntityManagerInterface the injection should be removed entirely.
The attached patch removes this injection and fixes the use of the deprecated entity.manager service as well.
Comment #14
TR CreditAttribution: TR commentedComment #15
BerdirI think I also did this in #2886622: Deprecate all EntityManager methods with E_USER_DEPRECATED but that is a massive patch, so happy to do a reroll there if we want to commit this first and separtely, removing services is always a bit icky, so might be better to do it as a dedicated and small patch.
There are ways to keep BC by removing the type hint but we didn't do that either in other cases that are very unlikely to be subclassed.
Comment #16
alexpott@Berdir I agree doing this in a separate issue makes things a bit easier. http://grep.xnddx.ru/search?text=CommentTypeDeleteForm&filename= is nice in that nothing looks like it is subclassing it.
Comment #17
alexpottCommitted d397f60 and pushed to 8.8.x. Thanks!