Remove stub entities, replace entity_create_stub_entity()

hm, I'm not so sure about this one. What's the point of having the stub entity complying to the EntityInterface if most of its method won't work anyway?

I think what would make more sense is to use the real entity classes + lazy load them. But lazy-loading should know it's actually an EntitySet or so, which takes care of multiple loading.

Obviously, lazy-loading just plays with using the methods. Still, it would be a big DX++ if there is no difference between the stub and the loaded entity at all. That means, the not yet loaded stubs would have to be registered in the entity controller as well... (But that could be done once we move the actual querying of EFQ to the controller where it should be - only the storage can know how to query).

Ok, getting back to the present. We'll need an intermediate solution, so what about going with

array($id, $vid, $bundle) instead of the stub? That is what entity_extract_ids() already returns, so converting should be rather easy then.

@fago, please see #1550454-18: Regression: Following taxonomy entity conversion, Taxonomy EFQ causes warnings and notices, EFQ currently relies on the concept of a stub entity.

I agree with fago that it would be less a hassle to just kill the concept of stub entity completely. Other then a couple of tests and some misguided code in file_field_update() (there is a patch somewhere to kill it), it's really only used in EFQ.

EFQ should support multiple "hydration" modes, including lazy loading, but in the meantime I'm happy with just returning array($id, $vid, $bundle) from EFQ.

I've unpostponed #1237636: Entity lazy multiple front loading so we can discuss lazy loaded entities some more.

#12: stub-entity-concept-removed-1551140-12.patch queued for re-testing.

Yeah, lazy-loading should be the long term approach. Short term I'd love to get rid of stub or partial entities anyway. Let's avoid incomplete entities being passed around, that just leads to bugs if some code expects a "normal" entity - what usually is a sane thing to do ;)

Thus, I'd prefer #12.

+++ b/core/modules/entity/entity.module
@@ -177,20 +178,20 @@ function entity_extract_ids($entity_type, $entity) {
+function entity_create_from_ids($entity_type, $ids) {

Although this one really doesn't make much sense. It's an API function for creating a new entity with pre-defined ids and bundles? While we already have enforceIsNew() I don't think we want to add this function?

Couldn't we do an private field api helper instead? So people don't see it and get confused, or even use it? Also, should we add an @todo to field_purge_data() to avoid passing partial entities around?

Bumping priority to major. This issue is blocking #1616930: Fix the field_test entity type, which is blocking #218755: Support revisions in different states and probably others.

Both @Damien and @fago seem to be in favor of #12 and the arguments make sense, so I'd +1 that.

Since this is blocking other issues, and the full conversion for #12 will require follow-up work, I'd even support the idea of committing a patch that's not picture-perfect here.

That said, I agree with the remark about entity_create_from_ids() in #17, and think

- we should make sure that the ->original change to field_field_update() still does what it is supposed to do.

- the change to TermStorageController could use a comment to explain why/when a taxonomy term could "suddenly" have no vid.

- the assignment of an array to a variable called $entity in EntityFieldQuery is bogus.

+++ b/core/modules/field/field.moduleundefined
@@ -1202,3 +1202,30 @@ function theme_field($variables) {
+ ¶
\ No newline at end of file

;)

-24 days to next Drupal core point release.

+++ b/core/modules/field/lib/Drupal/field/Tests/EntityPropertiesTest.phpundefined
@@ -44,7 +48,7 @@ class EntityPropertiesTest extends FieldTestBase {
+          $this->assertEqual($label, $entity->ftlabel, 'Entity rray) $entitwith label key returned correct label.');

typo

ITS GREEN! THANK YOU!

-25 days to next Drupal core point release.

I am fine with this change. Most of the time noone cares much about the values of $return we use the array_keys($return[$entity_type]) the rest are fringe edge cases if there's anything usable, it'll do.

thanks Berdir - the patch looks great! I've had a close look at it and found mostly nitpicks:

+    // bundle key. Only attempt to do this if a vocabulary id is available,
+    // which might not be the case when creating partial/incomplete entities.
     // @todo Move to Term::bundle() once field API has been converted
     //   to make use of it.
-    if (!isset($entity->vocabulary_machine_name)) {
+    if (!isset($entity->vocabulary_machine_name) && isset($entity->vid)) {

entity_create() defines that the bundle has to be always specified, what contradicts that change. Looking at the patch I wonder whether this is still necessary though? Looks like the bundle should be passed on as well.

+++ b/core/modules/entity/lib/Drupal/entity/EntityFieldQuery.php
@@ -710,20 +710,21 @@ class EntityFieldQuery {
+   *   results were found. The inner array values consist of an object
+   *   with the entity_id, revision_id and bundle properties. To traverse the

with should be at the upper line

+++ b/core/modules/field/field.module
@@ -1206,3 +1206,29 @@ function theme_field($variables) {
+ * This can be used to create an entity based on the the
+ * ids object returned by EntityFieldQuery.
+ *

strange new line in there.

+++ b/core/modules/field/lib/Drupal/field/Tests/BulkDeleteTest.php
@@ -182,10 +178,14 @@ class BulkDeleteTest extends FieldTestBase {
+       $this->assertEqual($this->entities[$id]->{$field['field_name']}, $entity->{$field['field_name']}, "Entity $id with deleted data loaded correctly");

This adds an extra, unnecessary space to the indentation of the last line.

+++ b/core/modules/field/tests/modules/field_test/field_test.storage.inc
@@ -325,7 +325,7 @@ function field_test_field_storage_query($field_id, $conditions, $count, &$cursor
+            $return[$row->type][$id] = (object)array('entity_id' => $row->entity_id, 'revision_id' => $row->revision_id, 'bundle' => $row->bundle);

missing space after (object)

ok, fixed the nitpicks and removed the taxonomy term creation change - let's see whether it's required.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.phpundefined
@@ -25,10 +25,11 @@ class TermStorageController extends DatabaseStorageController {
+    // bundle key. Only attempt to do this if a vocabulary id is available,

s/id/ID

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.phpundefined
@@ -25,10 +25,11 @@ class TermStorageController extends DatabaseStorageController {
+    // which might not be the case when creating partial/incomplete entities.

Can we agree on a terminology? partial/incomplete sounds rather partial and incomplete :)

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/EfqTest.phpundefined
@@ -44,6 +44,10 @@ class EfqTest extends TaxonomyTestBase {
+    $this->assertEqual($term->tid, $first_result->entity_id, 'Taxonomy term can be created based on the ids');

s/ids/IDs

I see. Although, I don't see how vocabularies as configuration would help us here?

The problem is that we've bundles that are not represented in the storage, so the right fix is imho
#1496612: Add bundle in the storage of taxonomy terms and also #1496614: Add bundle in the storage of comments for comments.

So what about adding a @todo to remove it once we've proper bundle columns so EFQ works?

yep, but the point is not that vocabularies become configuration, the point is that it makes vocabulary names their unique ID and so the 'vid' column of terms become their proper bundle storage column.

Thus #1496612: Add bundle in the storage of taxonomy terms and #1496614: Add bundle in the storage of comments need to be fixed anyway, regardless of how #1552396: Convert vocabularies into configuration continues.

There is already a @todo there about moving it to Term::bundle(), I guess we would replace that, we can't do both? :)

hm, you are right - the method already relies on the bundle to be there so moving to the method says that as well.

Alright, imho this is good to go then!

#38: stub-entity-concept-removed-1551140-38.patch queued for re-testing.

Committed and pushed to 8.x. This will need a change notice.

Also, I think it's odd that this function is prefixed with an underscore despite being called in many other places than field.module. So I'd love a follow-up that removed the underscore and clarified the comments about what the function is/isn't for.

Looks good to me. I agree there is no reason to advertise usage of a "private" (underscore-prefix) function.