While reviewing

#2546212: Entity view/form mode formatter/widget settings have no translation UI

I noticed a minor screw up. New code is being introduced in the patch which has to make use of deprecated methods in the entityManager namely getDefinitions() and getStorage()

this is because ConfigEntityMapper provides

$this->entityManager

not

$this->entityTypeManager

So this issue is to fix the deprecation so we are no longer railroaded into using outdated methods.

CommentFileSizeAuthor
#7 3031346-7.patch6.71 KBmartin107
#7 interdiff-3031346-2-7.txt972 bytesmartin107
#2 3031346-2.patch6.48 KBmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107 created an issue. See original summary.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Assigned: Unassigned » martin107
Status: Active » Needs review
FileSize
3031346-2.patch6.48 KB

Here is the first cut patch

a) pull in the preferred entity_type.manager, remove entityManager

b) fix up all classes that extend ConfigEntityMapper

c) cross fingers wait for testbot to complain.

To review my own patch

I am mildly concerned that the removal of $this->entityManager from the class might be seen as a breaking change by contrib.

Unless someone objects I am just going to ignore this concern as contrib if caught out by this should be able to adjust .. If anyone objects then the solution is to reinsert the entityManager back into the class and apply deprecated notices.

Anyway I want to hear what other think.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Issue summary: View changes

minor grammar errors corrected in the issue summary.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Assigned: martin107 » Unassigned
Issue summary: View changes

This patch looks complete to me.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Title: Prevent EntityManager railroading » Replace EntityManager usage in ConfigEntityManager
Status: Needs review » Needs work
Parent issue: » #2886622: Deprecate all EntityManager methods with E_USER_DEPRECATED 
+++ b/core/modules/config_translation/src/ConfigEntityMapper.php
@@ -24,9 +24,9 @@ class ConfigEntityMapper extends ConfigNamesMapper {
    * The entity manager.
    *
-   * @var \Drupal\Core\Entity\EntityManagerInterface
+   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
    */
-  protected $entityManager;
+  protected $entityTypeManager;

We have DeprecatedServicePropertyTrait so that subclasses could still access the entityManager property without breaking.

Also going for a simpler issue title that describes what this issue actually does ;)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Title: Replace EntityManager usage in ConfigEntityManager » Replace EntityManager usage in ConfigEntityMapper
martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff-3031346-2-7.txt972 bytes
3031346-7.patch6.71 KB

Thank you for the review.

I am glad that DeprecatedServicePropertyTrait solves all the concerns about internal properties details being used by contrib.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. I think this is a useful separate part to update. There's another issue for forms that's not yet RTBC, just in case you want to help with entity manager conversions: #2969109: Replace deprecated usages of entityManager in form classes. That (helping with these issues) would be very appreciated, but we need to be careful to not have too many overlapping issues with conflicts. I'm trying to keep the parent issue updated with the current focus.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 3031346-7.patch, failed testing. View results

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Reviewed & tested by the community

DrupalCI was unhealthy, latest patch is green again.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 3031346-7.patch, failed testing. View results

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Reviewed & tested by the community

DrupalCI was unhealthy again, latest patch is again green again.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed ad0506c and pushed to 8.7.x. Thanks!

diff --git a/core/modules/config_translation/src/ConfigEntityMapper.php b/core/modules/config_translation/src/ConfigEntityMapper.php
index b3270232c9..cdf07bfc63 100644
--- a/core/modules/config_translation/src/ConfigEntityMapper.php
+++ b/core/modules/config_translation/src/ConfigEntityMapper.php
@@ -80,7 +80,7 @@ class ConfigEntityMapper extends ConfigNamesMapper {
    *   The route provider.
    * @param \Drupal\Core\StringTranslation\TranslationInterface $translation_manager
    *   The string translation manager.
-   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_manager
+   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
    *   The entity manager.
    * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
    *   The language manager.

Fixed docs on commit.

  • alexpott committed ad0506c on 8.7.x 
    Issue #3031346 by martin107, Berdir: Replace EntityManager usage in...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.