Problem/Motivation

CEB::getTranslation keeps a translation cache under $this->translation for each initialized translation, however when we call getTranslation on the default translation the default translation is not put into the translation cache which leads to initializing an additional entity object translation when retrieving the default translation from a non-default translation.

Proposed resolution

When retrieving a translation put the current translation into the translation cache as well.

Remaining tasks

Reivew & Commit.

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hchonov created an issue. See original summary.

hchonov’s picture

Issue tags: +Novice
FileSize
913 bytes

The only possible test I can think of would be:

$entity = $storage->load();
$default_translation_spl_object_hash  = spl_object_hash($entity);
$this->assertEqulas($default_translation_spl_object_hash, spl_object_hash($entity->getTranslation('non-default')->getTranslation('default')));

Does such a test make sense?

hchonov’s picture

The last submitted patch, 2: 2839089-2.patch, failed testing.

The last submitted patch, 3: 2839089-3-test-only.patch, failed testing.

hchonov’s picture

Issue tags: +Performance, +D8MI

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hchonov’s picture

Issue tags: +DevDaysSeville
tstoeckler’s picture

Looked at this. I think in general, this makes a lot of sense, as it's a harmless optimization.

I found the following code, though, a couple lines further up in ContentEntityBase::getTranslation():

    // Populate entity translation object cache so it will be available for all
    // translation objects.
    if ($langcode == $this->activeLangcode) {
      $this->translations[$langcode]['entity'] = $this;
    }

So I think we should consolidate. It seems as though there is no reason not to always cache $this in $this->translations, so maybe we can replace the above + the hunk in the patch simply with:

if (!isset($this->translations[$this->activeLangcode])) {
  $this->translations[$this->activeLangcode] = $this;
}

Thoughts?

hchonov’s picture

FileSize
2.96 KB
1.27 KB

@tstoeckler, yes that makes sense :). Thank you for the suggestion.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

arunkumark’s picture

Assigned: Unassigned » arunkumark
Status: Needs review » Needs work
arunkumark’s picture

Assigned: arunkumark » Unassigned
Status: Needs work » Needs review
FileSize
2.96 KB

Hi All,

I have re-rolled the patch for Drupal 8.5.x version.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

anmolgoyal74’s picture

Hi All,
Here is the re-rolled patch for Drupal 8.6.x version.

tstoeckler’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -846,9 +846,9 @@ public function getTranslation($langcode) {
    +     }
    

    Wrong indentation here.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityTranslationTest.php
    @@ -1012,5 +1013,33 @@ public function testTranslationStatus() {
    +    $entity->addTranslation($translation_langcode)
    +      ->save();
    

    Minor, but this does not need to be broken into two lines.

anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
3.11 KB

Hi All,
Here is the updated re-rolled patch for Drupal 8.6.x version.

Status: Needs review » Needs work

The last submitted patch, 17: initializes-object-2839089-17.patch, failed testing. View results

anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
3.11 KB
tstoeckler’s picture

Status: Needs review » Needs work

The patch does not fix #16 and instead introduces some whitespace problems.

leolandotan’s picture

Assigned: Unassigned » leolandotan

I'll work on this.

leolandotan’s picture

Assigned: leolandotan » Unassigned
Status: Needs work » Needs review
FileSize
2.97 KB
2.71 KB

As per the comment from #16, I have applied the fix on coding standards and some additional ones as well.

Hope everything is in order.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Perfect, thank you!

alexpott’s picture

Category: Bug report » Task

This is a task. The current behaviour is not a bug it is just not the most efficient and whilst I don't doubt there is a performance increase I'd be surprised if it is enough to make this a bug.

alexpott’s picture

Adding @tstoeckler to commit credit for reviews.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed cebca29 and pushed to 8.6.x. Thanks!

  • alexpott committed cebca29 on 8.6.x
    Issue #2839089 by hchonov, anmolgoyal74, leolando.tan, arunkumark,...

Status: Fixed » Closed (fixed)

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