CommentFileSizeAuthor
#82 entity-bc-removal-1983554-82.patch72.51 KBeffulgentsia
#80 entity-bc-removal-1983554-80.interdiff.txt933 bytesplach
#80 entity-bc-removal-1983554-80.patch72.54 KBplach
#72 entity-bc-removal-1983554-72.patch72.34 KBeffulgentsia
#72 interdiff.txt4.1 KBeffulgentsia
#64 entity-bc-removal-1983554-64.interdiff.do_not_test.patch1.38 KBplach
#64 entity-bc-removal-1983554-64.patch72.31 KBplach
#62 entity-bc-removal-1983554-62.patch72.69 KBplach
#59 entity-bc-removal-1983554-59.interdiff.do_not_test.patch1.17 KBplach
#59 entity-bc-removal-1983554-59.patch73.34 KBplach
#55 entity-bc-removal-1983554-55.interdiff.do_not_test.patch678 bytesplach
#55 entity-bc-removal-1983554-55.patch72.87 KBplach
#53 entity-bc-removal-1983554-53.interdiff.do_not_test.patch1.25 KBplach
#53 entity-bc-removal-1983554-53.patch72.85 KBplach
#48 entity-bc-removal-1983554-48.patch72.19 KBeffulgentsia
#48 interdiff.txt630 byteseffulgentsia
#46 entity-bc-removal-1983554-46.patch72.12 KBeffulgentsia
#46 interdiff.txt5.29 KBeffulgentsia
#45 entity-bc-removal-1983554-45.patch70.87 KBeffulgentsia
#45 interdiff.txt822 byteseffulgentsia
#44 entity-bc-removal-1983554-44.interdiff.do_not_test.patch757 bytesplach
#44 entity-bc-removal-1983554-44.patch70.88 KBplach
#42 entity-bc-removal-1983554-42.patch70.14 KBeffulgentsia
#38 entity-bc-removal-1983554-38.interdiff.do_not_test.patch4.09 KBplach
#38 entity-bc-removal-1983554-38.patch70.75 KBplach
#36 entity-bc-removal-1983554-36.patch68.11 KBplach
#36 entity-bc-removal-1983554-36.interdiff.do_not_test.patch9.41 KBplach
#28 entity-bc-removal-1983554-28.patch60.87 KBplach
#28 entity-bc-removal-1983554-28.interdiff.do_not_test.patch4.43 KBplach
#24 entity-bc-removal-except-storage-1983554-24.patch57.82 KBBerdir
#22 entity-bc-removal-except-storage-1983554-22.patch26.65 KBBerdir
#22 entity-bc-removal-except-storage-1983554-22-interdiff.txt4.31 KBBerdir
#21 entity-bc-removal-except-storage-1983554-21.patch22.37 KBeffulgentsia
#21 interdiff.txt1.45 KBeffulgentsia
#19 entity-bc-removal-except-storage-1983554-19.patch23.13 KBeffulgentsia
#19 interdiff.txt4.11 KBeffulgentsia
#16 entity-bc-removal-except-storage-1983554-16.patch20.04 KBeffulgentsia
#13 entity-bc-removal-except-storage-1983554-13.patch21.92 KBeffulgentsia
#11 ninja-removal-1983554-11.patch528 bytesBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Issue tags: +Performance, +Entity Field API

tagging

webchick’s picture

Priority: Major » Critical

I don't see how we can ship with both, so raising to critical.

Happy to see this issue, though. :D

webchick’s picture

Status: Postponed » Active

Hm. Looks like both dependent issues have been resolved, can we do this now? (Or was it already done in the past 8 weeks and I missed it? :D)

catch’s picture

It wasn't done in the past eight weeks. I'm pretty sure there's dependent issues not listed in the summary (modules still using the bc layer for nodes, field items, config entities?) that aren't fully done yet.

plach’s picture

We need at least to complete the conversion of nodes and users and the field storage stuff.

Berdir’s picture

Yes, there are some things left.

* Remove BC from nodes: #1939994: Complete conversion of nodes to the new Entity Field API
* Remove BC from users: #2017207: [meta] Complete conversion of users to Entity Field API
* Convert menu_link #1842858: [PP-1] Convert menu links to the new Entity Field API
* Convert field storage code to use NG, first step #1497374: Switch from Field-based storage to Entity-based storage, then follow-up to update it to use the new Code, possibly together with merging the NG database storage controller together.

Those issues are already all critical on their own. What's not yet completely clear is what will happen to config entities. But I currently don't really see a solution make them NG, so we might move the complex data interface down to content entity interface and merge EntityNG into ContentEntityBase. #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface is a first step towards moving interfaces/methods down to ContentEntityInterface/Base.

There's some clean-up stuff to do then, the new Field Type stuff still supports BC entities, but that is no longer necessary, merge some NG controllers with their non-NG pendant or rename to a Content-version.

webchick’s picture

This will obviously be a BC break, but we need to do this before releasing. Tagging.

xjm’s picture

@webchick added these tags in #8, but it looks like Tag Monster is out in force today.

effulgentsia’s picture

Berdir’s picture

Status: Active » Needs review
FileSize
528 bytes

Ninja-style removal of the BC decorator ;)

This obviously breaks any test that will attempt to save or load field values.

Additionally, the following allows to load field item values, but only for the default language. With that, I'm able to look at a rendered node, so it might be easier to the remove BC deorator than I thought. But there will for sure be special cases that I'm not seeing yet ;)

diff --git a/core/modules/field_sql_storage/field_sql_storage.module b/core/modules/field_sql_storage/field_sql_storage.module
index 3e5e25f..bff8dd5 100644
--- a/core/modules/field_sql_storage/field_sql_storage.module
+++ b/core/modules/field_sql_storage/field_sql_storage.module
@@ -411,7 +411,7 @@ function field_sql_storage_field_storage_load($entity_type, $entities, $age, $fi
         }
 
         // Add the item to the field values for the entity.
-        $entities[$row->entity_id]->{$field_name}[$row->langcode][] = $item;
+        $entities[$row->entity_id]->{$field_name}[] = $item;
         $delta_count[$row->entity_id][$row->langcode]++;
       }
     }

Status: Needs review » Needs work

The last submitted patch, ninja-removal-1983554-11.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
21.92 KB

Curious if we can at least get this far (only use BC for storage hooks).

Status: Needs review » Needs work

The last submitted patch, entity-bc-removal-except-storage-1983554-13.patch, failed testing.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
@@ -273,31 +273,10 @@ public function validate(array $form, array &$form_state) {
+    foreach ($entity as $field_name => $field) {
+      $field_violations = $field->validate();

Interesting. This probably doesn't explode as config entities (have to) implement IteratorAggregable but getIterator() returns an empty iterator, so this never returns anything. Because they would return raw scalar values, which would obviously explode.

That's a pretty crazy thing to rely on, so it probably makes sense to get #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface in first, as that moves the validate() stuff down to ContentEntityFormController.

And nice, not too many fails remaining.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
20.04 KB

Just a reroll. Still need to fix the failures and address #15.

Status: Needs review » Needs work

The last submitted patch, entity-bc-removal-except-storage-1983554-16.patch, failed testing.

Berdir’s picture

Issue tags: +sprint

Tagging for rocketship focus

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
4.11 KB
23.13 KB

This fixes the failures, but doesn't yet address #15.

Status: Needs review » Needs work

The last submitted patch, entity-bc-removal-except-storage-1983554-19.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.45 KB
22.37 KB

Still just trying to get a green patch here before addressing #15. The addition of getBCEntity() to field_language() in #19 wasn't aggressive enough, causing the 2 new failures. This makes it more aggressive. Would be nice to fix that whole function's pipeline to work with EntityNG, but I don't know how to.

Berdir’s picture

Re-roll and started with field storage. Field language handling is not correct yet, but it seems to be able to storage and load stuff at least for single-language entities.

Status: Needs review » Needs work

The last submitted patch, entity-bc-removal-except-storage-1983554-22.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
57.82 KB

Yeah, assumed something like that :)

- The file usage stuff is I think dealt with in #2015697: Convert field type to typed data plugin for file and image modules, so let's get that in first.
- field cache seems to be a bit strange, maybe we need to change that to only cache values?
- The language stuff might require #2019055: Switch from field-level language fallback to entity-level language fallback, not sure. Needs @plach I guess.

Haven't fixed any tests, but the attached patch removes all BC related code that I could find. Interdiff isn't very useful, mostly just removed code.

The @todo in EntityNG::__get() is a bit unfortunate, there's code that now relies on this, e.g. render controller do things like $entity->content['bla'] = blub. Not sure if we want to break that.

Status: Needs review » Needs work

The last submitted patch, entity-bc-removal-except-storage-1983554-24.patch, failed testing.

yched’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
    @@ -655,8 +646,8 @@ protected function doSaveFieldItems(EntityInterface $entity, $update) {
    -      foreach ($field_langcodes as $langcode) {
    -        $items = (array) $entity->{$field_name}[$langcode];
    +      foreach ($entity->getTranslationLanguages() as $langcode => $language) {
    +        $items = (array) $entity->getTranslation($langcode)->{$field_name}->getValue();
    

    Yay !
    Is the (array) cast still needed ? Field::getValue() can return NULL ?

  2. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Tests/ConfigTestTranslationUITest.php
    @@ -67,9 +67,8 @@ function testTranslationUI() {
    -    $translation = $this->getTranslation($entity, $default_langcode);
         foreach ($values[$default_langcode] as $property => $value) {
    -      $stored_value = $this->getValue($translation, $property, $default_langcode);
    +      $stored_value = $entity->{$property};
    

    I'm very likely missing something, but this looks weird, you don't fetch $entity->getTranslation($entity, $default_langcode) ?

effulgentsia’s picture

Re #26.2, that test is weird to begin with: it's testing a content_translation functionality of a config entity. The change to that test is to make it effectively the same as what HEAD is doing in the base class for non-NG, now that the base class removed that check. I wonder if we want to remove that test entirely in #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface, or figure out what its real purpose is.

plach’s picture

Status: Needs work » Needs review
FileSize
4.43 KB
60.87 KB

This should fix some failures: field cache was broken wrt field language, it was storing just one language. I tried also to provide a quick fix for field_language(), the actual fix is #2019055: Switch from field-level language fallback to entity-level language fallback.

@effulgentsia:

I wonder if we want to remove that test entirely in #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface, or figure out what its real purpose is.

IIRC that test was introduced right after committing the first Content Translation patch, because it was erroneously trying to store translation metadata also for config entities. With #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface CT will officially support only content entities but it might make sense to keep that test to check that CT is not trying to support stuff it cannot. We can remove that test once #1916790: Convert translation metadata into regular entity fields is in, since the original bug will no longer be reproducible.

Status: Needs review » Needs work

The last submitted patch, entity-bc-removal-1983554-28.patch, failed testing.

Berdir’s picture

Thanks, getting there!

The EntityResolverTest is ugly, that's an endless loop because the entity apparently references itself and getValue(TRUE) there causes it to be loaded again and again. And we don't want to cache the computed entity referenced, but we do want to cache the computed processed texts for example.

Maybe introduce a new method on Field(Item)Interface, something like getCache() or getCacheValues() ? And introduce a new cacheable key for property definitions?

plach’s picture

What about implementing Serializable in the entity reference field?

Edit: scratch that, probably it would quickly become more complex than going with #30

effulgentsia’s picture

I actually agree with the initial suggestion in #31. Rather than introducing a new made up method name, like getCacheValue(), I think Field(Item)Interface should extend Serializable.

effulgentsia’s picture

Oops. I think I just went through the same process that plach did when he reconsidered #31. Serializable must return a string, so no, that's not what we want. getCacheableValue() makes sense then.

+++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.php
@@ -59,7 +59,9 @@ protected function loadFieldItems(array $entities, $age) {
           foreach ($cache[$cid]->data as $field_name => $values) {
-            $entity->$field_name = $values;
+            foreach ($values as $langcode => $items) {
+              $entity->getTranslation($langcode)->$field_name = $items;
+            }
           }

This should be $entity->getTranslation($langcode)->$field_name->setValue($items);, then, right? Or do we need a separate method, like setCacheableValue()?

Berdir’s picture

$field_name = $items calls __set(), which in turn calls setValue(), so either one works.

Serializable is actually broken/weird with 5.4/5.3, so it would have to be __sleep(). That it's a string isn't that relevant, the bigger problem is that after unserializing it again, it's again a field object, and when you do $field_name = $items, it still calls setValue(). That checks for typed data interface and calls getValue(), without TRUE, which means without computed properties.

So we'd have to call getValue(TRUE) or getCacheableValue() on the object we get back from the cache.. which would be useless, we'd better just store that in the first place :)

yched’s picture

Yes, I don't think we want to store serialized FieldField/Item objects in the cache (and definitely not including full serialized referenced entities ?). Wouldn't it kill the optimization of not creating those objects on a simple entity load ?

Does text 'processed' really have to be a 'computed' property ?
We'd need to make sure it's always present and to-date with the raw text values, but if it was a regular property autopopulated on load / read from cache / value change, then maybe we could stick with "cached values do not include computed properties" ?

plach’s picture

Status: Needs work » Needs review
FileSize
9.41 KB
68.11 KB

This should fix comment translation tests and entity translation API tests. Actually the root cause for both failures were translations not being properly handled during object cloning. Somehow the BC decorator was masking this. I also needed to fix more stuff with language fallback: hopefully I didn't break anything, otherwise it would probably better to get #2019055: Switch from field-level language fallback to entity-level language fallback in first.

This patch also adds a small optimization for field cache handling by instantiating the translation objects fewer times.

Status: Needs review » Needs work

The last submitted patch, entity-bc-removal-1983554-36.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
70.75 KB
4.09 KB

This should fix more failures. We should be left only with file-related failures which should be fixed by #2015697: Convert field type to typed data plugin for file and image modules and the entity reference cache issue above.

plach’s picture

Berdir suggested in IRC to address the field cache issue separately. Created #2083785: Add support for determining which field properties are cacheable, although I probably won't have time to work on it.

This is now blocked on that one and #2015697: Convert field type to typed data plugin for file and image modules.

Status: Needs review » Needs work

The last submitted patch, entity-bc-removal-1983554-38.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

We should be ready for review as the remaining failures should be fixed by the issues above.

effulgentsia’s picture

Status: Needs review » Needs work

The last submitted patch, entity-bc-removal-1983554-42.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
70.88 KB
757 bytes

Fixed the new failure.

effulgentsia’s picture

And just for fun, let's see if anything fails if we simply decide to not cache the processed text, in order to not hold this issue up on #2083785: Add support for determining which field properties are cacheable.

effulgentsia’s picture

This addresses #26 (incorporating the reply of #28), and some lines I found cumbersome from field.multilingual.inc.

I reviewed the rest of the patch, and it all looks great to me. I'd be comfortable with this being RTBC'd so long as someone approves the #45 interdiff and this one.

Status: Needs review » Needs work

The last submitted patch, entity-bc-removal-1983554-46.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
630 bytes
72.19 KB

I guess the answer to #26.1 is yes.

Status: Needs review » Needs work

The last submitted patch, entity-bc-removal-1983554-48.patch, failed testing.

plach’s picture

I'd revert the change to field.multilingual.inc: since we are trying to get rid of it altogether in #2067079: Remove the Field Language API, I'd avoid adding more functions to it. Otherwise we'll have to reroll #2019055: Switch from field-level language fallback to entity-level language fallback to remove _field_translated_value_exists() right after this is committed.

yched’s picture

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -575,7 +575,6 @@ protected function doLoadFieldItems($entities, $age) {
       $results = $this->database->select($table, 't')
         ->fields('t')
         ->condition($load_current ? 'entity_id' : 'revision_id', $ids, 'IN')
-        ->condition('langcode', field_available_languages($this->entityType, $field), 'IN')
         ->orderBy('delta')
         ->condition('deleted', 0)
         ->execute();

Why do we need to remove this condition ?
The list of languages returned by field_available_languages() might vary over time - e.g. if the field changes from translatable to untranslatable, + that list has an alter hook currently, + more generally I'm not sure what happens when a language gets removed from the system...
This condition ensures that we only populate the entity with languages that are valid for the current runtime context.

plach’s picture

Yeah, right, we should limit the query to the available entity translations.

plach’s picture

Status: Needs work » Needs review
FileSize
72.85 KB
1.25 KB

This restores the previous behavior with the exception of hook_field_available_languages_alter(), which I don't think we should support anymore, as I was saying in #2067079: Remove the Field Language API. We should be ready if this is green.

Status: Needs review » Needs work

The last submitted patch, entity-bc-removal-1983554-53.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
72.87 KB
678 bytes

Let's try this.

Status: Needs review » Needs work
Issue tags: -Performance, -API change, -sprint, -Entity Field API, -Approved API change

The last submitted patch, entity-bc-removal-1983554-55.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

#55: entity-bc-removal-1983554-55.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance, +API change, +sprint, +Entity Field API, +Approved API change

The last submitted patch, entity-bc-removal-1983554-55.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
73.34 KB
1.17 KB

This should restore the field storage language behavior presently implemented in HEAD. Revising it is being discussed in #2076445: Make sure language codes for original field values always match entity language regardless of field translatability.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -569,15 +570,17 @@ protected function doLoadFieldItems($entities, $age) {
+      $langcodes = $field['translatable'] ? $all_langcodes : array(Language::LANGCODE_NOT_SPECIFIED);
-        ->condition('langcode', field_available_languages($this->entityType, $field), 'IN')
+        ->condition('langcode', $langcodes, 'IN')

In HEAD, field_available_languages() also checks that the entity type is translatable, but the new code does not. Why?

Status: Needs review » Needs work

The last submitted patch, entity-bc-removal-1983554-59.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
72.69 KB

Rerolled

@effulgentsia:

In HEAD, field_available_languages() also checks that the entity type is translatable, but the new code does not. Why?

Actually it is checking whether there is any translation handler, which is a deprecated concept. Currently we have a BC layer in place which falls back to checking whether the entity type is translatable (the closest thing we could come up with in D8), but that is going away soon. That check is now superfluous as a field cannot be made translatable if the related entity is not translatable. Even if some module worked around this limitation, field translatability would still be the only thing that matters to determine which languages should be retrieved.

Status: Needs review » Needs work

The last submitted patch, entity-bc-removal-1983554-62.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
72.31 KB
1.38 KB

Fixed field items saving. Hopefully this should be green again.

effulgentsia’s picture

Thanks. I'm tempted to RTBC this, but I'll give it 12 hours or so for yched to double check.

Berdir’s picture

Almost back from holidays :)

I'm fine with getting this in without the cache for computed properties *but* that means we need to make [#/2083785] critical (and get an API change approval in case we need to change things) because that will introduce a *serious* performance regression, that we already fixed once last week.

I'd also love to see a follow-up issue to improve the entity loading/object creation. If we manage to build up a single $values array consisting of both base and configurable fields and then just call new $class($values), that would be awesome, because then we'd really only needed to create field objects for the values that are used. That was the idea of all that lazy loading stuff but as long as we do $entity->$field = $value, we have to instantiate all that stuff. I guess that will only work when we have an entity cache, no just a configurable field cache. Thinking about that part, could we possibly just generalize that a bit to base fields too? Then we could pretty easily cache all those things too...

I was also surprised to not see the tests fail that I added for the processed caching, but that's because I only tested the cache where the cache already exists, I'm not validating that the cache is written correctly. We need to improve this in the other issue.

yched’s picture

Will try to review asap.

follow-up issue to improve the entity loading/object creation

Big +1 on that. The current code flow between attachLoad() / mapFromStorageRecords() / attachPropertyData() is very convoluted and makes weird switching between entities as loaded array records and entities as real objects.

And +1 on working on a "base + configurable" fields data cache, the current situation is absurd (all FieldItem classes can have a prepareCache() method, but it's only invoked for configurable fields).
I haven't really looked into how easy it would be, partly because of the above (loading of base field data is confusing right now). I've mentioned that as one of the points to discuss during the pre-conf sprint meetings in Prague.

plach’s picture

+1 on #66 and #67!

I am already trying to clean that up a little in #2057401: Make the node entity database schema sensible, but we will a lot more :)

yched’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Doesn't seem to apply anymore
Sorry, my bad

@plach: you shouldn't name your interdiffs "interdiff-something.patch", it messes with drush_iq. interdiff.txt usually works fine ;-)

yched’s picture

yched’s picture

Issue tags: -Needs reroll
  1. +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
    @@ -569,15 +570,17 @@ protected function doLoadFieldItems($entities, $age) {
    +    $all_langcodes = array_keys(language_list());
    

    In doLoadFieldItems() we load values for langcodes array_keys(language_list()) (i.e all languages known by the system)
    In doSaveFieldItems() we save values for langcodes array_keys($entity->getTranslationLanguages())

    Why don't we also rely on $entity->getTranslationLanguages() on load ?
    Is it because ->getTranslationLanguages() returns langcodes based on what is found in the $entity and thus cannot be used while we are populating the entity ? In that case, a comment might be useful.

  2. +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
    @@ -655,8 +648,12 @@ protected function doSaveFieldItems(EntityInterface $entity, $update) {
    -      foreach ($field_langcodes as $langcode) {
    -        $items = (array) $entity->{$field_name}[$langcode];
    +      $langcodes = $field['translatable'] ? array_keys($entity->getTranslationLanguages()) : array(Language::LANGCODE_NOT_SPECIFIED);
    +      foreach ($langcodes as $langcode) {
    +        $items = $entity->getTranslation($langcode)->{$field_name}->getValue();
    +        if (!isset($items)) {
    +          continue;
    +        }
    

    Personal taste I guess, but I'd rather avoid the "continue" and either switch back to casting to an ampty array, or wrapping the code in an if ($items = ...) {do stuff}
    Maybe it's just me, feel free to ignore.

  3. +++ b/core/lib/Drupal/Core/Entity/EntityNG.php
    @@ -817,7 +793,15 @@ public function __clone() {
    +      // Ensure the translations array is actually cloned by removing the
    +      // original reference and re-creating its values.
           $this->clearTranslationCache();
    +      $translations = $this->translations;
    +      unset($this->translations);
    +      // This will trigger the magic setter as the translations array is
    +      // undefined now.
    +      $this->translations = $translations;
    

    This looks really weird, but no better suggestion on my side, this area is a bit over my head :-/

  4. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Tests/ConfigTestTranslationUITest.php
    @@ -54,23 +54,24 @@ protected function getNewEntityValues($langcode) {
    +   * Overrides \Drupal\content_translation\Tests\ContentTranslationUITest::::testTranslationUI().
    

    That's a lot of ':'s ;-)

  5. +++ b/core/modules/field/field.multilingual.inc
    @@ -351,3 +351,16 @@ function field_language(EntityInterface $entity, $field_name = NULL, $langcode =
    +  $field->filterEmptyValues();
    +  $value = $field->getValue();
    +  return !empty($value);
    

    return $field->isEmpty() ?

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
4.1 KB
72.34 KB

This addresses #71's 2, 4, and 5. I don't know what to do about 1 and 3.

webchick’s picture

Just one observation/question, not a review:

index d0b6b4c..d092b2c 100644
+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -70,13 +70,6 @@ class EntityNG extends Entity {

I'm surprised that this class still exists after this patch. I would've expected it to become just "Entity." When/where is that happening? We can't ship D8 with something called "EntityNG" IMO.

Berdir’s picture

Yes, I said a while back that this will happen when we will remove EntityBC. In short, EntityNG as a name will not stay, but it won't be removed in this issue, that will happen in #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface.

To explain, back when I said that, we assumed that all entity types, including config entities will be NG'ified. However, nobody had time to work on it so it didn't happen. Given the number of config entities that we have by now, that would be a crazy effort. But, we no longer need EntityBC, which is only used to access already converted entity types in the old way. EntityBC was never used with config entities. The only remaining database entity that is not converted to EntityNG is menu_link, and that is not fieldable currently. That means we can safely kick the EntityBC code out.

What the referenced issue will do is rename EntityNG to ContentEntityBase (and slightly change the meaning of content entity, also making e.g. users a content entity), which means that a large number of functionality will only work with entity types that implement ContentEntityInterface: having configurable fields, content translation (makes kinda sense ;)), being supported by rest.module (at least the current implementation that we have in core, it should be possible to integrate with config entities specifically too) and edit.module.

That's not perfect, it would be nice if config entities would support the same API, but config entities are very different to content entities in many ways, so I think it's a reasonable compromise, the only one that I see. That's to be discussed in the referenced issue if necessary, just wanted to explain it.

webchick’s picture

Thanks, that's very helpful. I'll follow that issue, too.

effulgentsia’s picture

Any chance of this issue being RTBC'd without addressing #71.1 and .3, and punting those to follow ups?

yched’s picture

Status: Needs review » Reviewed & tested by the community

Agreed, let's do that.

@plach, could you check #71 1. and 3. and see if they deserve followups ?

webchick’s picture

Issue tags: +needs profiling
32 files changed, 106 insertions, 1011 deletions.

This is absolutely impressive work, folks. Thank you SO much!

Tentatively tagging "needs profiling." Not to block a commit, but because I'd love to figure out the performance increase we gain by removing all of that code.

Just a warning that I'm in the process of committing a bunch of stupid and pedantic DX clean-up patches today just to get them out of the way before DrupalCon which might break this one (hope not, but...) So maybe hold off on any re-rolls until tomorrow.

Berdir’s picture

Not exactly sure how all the translation languages stuff plays together, but I also think it's related to the the actual field values, we need those before we know what translations we have. A comment might make sense but I'm not sure what we could write that would also make sense in the final code and not just explain the change in the patch.

As discussed in IRC, there's not much to profile here. This issue actually introduces a cache related performance regression, so might be slower than HEAD with a good amount of content on a page (due to slow check_markup() calls). So a before/after comparison will have a lot of noise in it and we can't say something like it's now 5% faster. Especially because most calls already have been converted to use EntityNG directly apart from field storage and we're mostly just removing dead code.

The main reason that this is relevant for performance is that it allows to do useful profiling again as this is the final part of removing a lot of magic-on-top-of-magic calls that itself does another calls (e.g., language() used to be called *a lot*), so we can actually look at how we use it and how we can improve it. One obvious example (already pointed out above, needs an issue) is the whole entity loading mess that currently hijacks a lot of the lazy-loading logic that is supposed to speed EntityNG up.

plach’s picture

Why don't we also rely on $entity->getTranslationLanguages() on load ?

The reason is that we are loading field values for multiple entities, hence we cannot limit the query to specific languages because the available translations might be different for each entity. Added a comment to clarify this.

This looks really weird, but no better suggestion on my side, this area is a bit over my head :-/

When cloning a translation object, the EntityNG::translations array is a reference (see EntityNG::initializeTranslation()). This means that it would be shared between the original object and its clone, which would cause weird behaviors. The only way I found to get an actual clone of the original array is unsetting the reference and setting the translations array back as an actual value afterwards.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

effulgentsia’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
72.51 KB

Straight reroll.

effulgentsia’s picture

Issue tags: -Needs reroll

Removing "Needs reroll" tag.

catch’s picture

Title: Remove BC-mode from EntityNG » Change notice/follow-up: Remove BC-mode from EntityNG
Priority: Critical » Major
Status: Reviewed & tested by the community » Active

Committed/pushed to 8.x, thanks!

I'm a bit confused which issue fixes the check_markup() caching regression, is it the computed properties one?

Not sure if this needs a change notice, maybe a short one for contrib entities which ought to completely break now if not updated?

Agree with the ContentEntityBase stuff etc. fwiw.

yched’s picture

effulgentsia’s picture

That's not perfect, it would be nice if config entities would support the same API, but config entities are very different to content entities in many ways, so I think it's a reasonable compromise, the only one that I see.

FWIW, I think that situation is completely ok. In many other systems, "entity" just means something that is persisted and is identifiable. ConfigEntityBase and ContentEntityBase then layer additional capabilities on top of basic CRUD, each one optimized for its use case and storage. Makes total sense, I think. Perhaps some further unification will be possible in D9, but I don't see the lack of that unification in D8 as bad at all.

tstoeckler’s picture

yched’s picture

Note: the changes in DatabaseStorageController::doSaveFieldItems() broke installation of the 'standard' profile in languages other than english - #2091523: Install in foreign language fails with integrity constraint violation on 'user_picture_target_id'

Berdir’s picture

Title: Change notice/follow-up: Remove BC-mode from EntityNG » Remove BC-mode from EntityNG
Priority: Major » Critical
Status: Active » Fixed

I don't know what do document here. I don't think we have an change notices about the BC decorator and the change notices that we created already use the new API's.

Unless contrib modules did a manual getBCEntity(), they already didn't see the BC decorator anymore for a while now.

Berdir’s picture

Issue tags: -sprint

Those sprint tags..

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