Problem/Motivation
That function currently is primarily used in EFQ to return an array of information that you can then use to actually load an entity. It is the counterpiece to entity_extract_ids() which returns array($id, $revision_id, $bundle), this function accepts these as arguments to create a minimal entity object. Both functions are bandaids because 7.x does not have a generic way to identify an $entity object.
During the initial entity class patch, entity_create_stub_entity() has been changed to internally call entity_create() which now returns a entity class. This can lead to nasty issues, see for example #1550454: Regression: Following taxonomy entity conversion, Taxonomy EFQ causes warnings and notices. Therefore, that change is currently reverted in that issue to resolve the unecessary regression.
Proposed resolution
During a discussion in IRC, I proposed to introduce a EntityStub class to replace that stub function in the long-term, with the following goals:
- Making it explicit that this is just a stub and *not* an actual entity. Among other things, it could throw an exception when trying to call save() on it.
- To provide an easy way to actually load the represented entity, e.g. entity_load_stub($stub), entity_load_stub_multiple($stubs) or $stub->load() (would not support multiple)
Remaining tasks
Discuss, patch, commit.
API changes
Obviously, the stub function would be removed and the new class introduced :)
Comments
Comment #1
BerdirI'll try to get something started here. This is actually a blocker to get entity_extract_ids() out...
Comment #2
BerdirOh, this is fun.
So this is a blocker to remove entity_extract_ids() because we need to be able to call id() and stuff on stub entities. At the same time need these stub entities to continue working with entity_extract_ids() which expects public properties based on the entity info.
So, this patch get's the thing started:
- entity_create_stub_entity() stays, gets the same arguments.
- Creates a EntityStub object instead.
- EntityStub implements EntityInterface
- Theoretically, EntityStub could lazy-load a lot of things on-demand, like label(). Not sure if that's good odea or not.
- To have both id() and entity_extract_ids(), working, I've added setStubId($property_name, $id) so that we can get the id without having to fallback on entity_type. We could alternatively call entityInfo() in id().
- Already starts to following the new suggested version instead of revision naming scheme.
The EFQ tests passed, let's see about the rest.
Thoughts?
Comment #3
BerdirHad another idea:
We could patch entity_extract_ids() to call id(), versionId() and bundle() if it's an EntityInterface. Then we don't need that magic. But for that, we first need to get the versionId() in.
Comment #4
fagohm, 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.
Comment #5
tim.plunkett@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.
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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.Comment #7
BerdirOk, so the issue that Damien meant is #985642: Remove file_attach_load() from file_field_update().
Let's try to get this one in then, and then we should be able to remove those stub entity stuff completely and we can then later on think about fancy lazy loading entities. Sounds like a nice plan to me.
Comment #8
BerdirField tests also use stub entities. I think we should change them to use one of the test entities that we have now.
Comment #9
BerdirOk, that's not exactly trivial, but here is a patch.
- EFQ now returns simple numerical arrays
- Removed stub keyword from comments, function, replaced with "array of entity ids" (EFQ) or partial entities (see below)
- entity_create_stub_entity() replaced with entity_create_from_ids(() which does call entity_create() and is only used on demand, when really required.
- The main case where it is required is field_purge_data(), which specifically only wants an entity object consisting of the ids + the deleted field. entity load does *not* work there, because it is possible that the entity was already deleted.
- Using entity_create() in a field_test function that creates an entity.
- Rename and adapt _generateStubEntities() to convertToPartialEntities() as that's the only purpose of it, it creates the same partial entity that field_purge_data() does, used for hook call validation.
- Contains #985642: Remove file_attach_load() from file_field_update() (someone wants to RTBC that one?), currently.
Comment #11
BerdirOh, almost forgot: This also adds a simplified version of the discussed fix for taxonomy terms so that entity_create_from_ids() works for them. With tests.
Comment #12
BerdirRe-roll after PSR-0 changes.
Comment #13
catchI've unpostponed #1237636: Entity lazy multiple front loading so we can discuss lazy loaded entities some more.
Comment #14
fago#12: stub-entity-concept-removed-1551140-12.patch queued for re-testing.
Comment #16
BerdirI can do a re-roll easily, but what's really required here is a decision whether we want to continue with #2 or #12. Opinions? Both are just a temporary solution to be able to continue with the removal of entity_extract_ids(), $type arguments and so on.
I think we all agree that we want to be able to return lazy loading entities from EFQ in the long term but that's much more involved than either of these two patches.
Comment #17
fagoYeah, 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.
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?
Comment #18
jstollerBumping 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.
Comment #19
BerdirI'm fine with moving that as a private, hopefully temporary function to field.module. Can do a re-roll of that.
Any other opinions in regards to #16?
Comment #20
sunBoth @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.
Comment #21
BerdirChanges.
- Renamed entity_create_from_ids() to _field_create_entity_from_ids(), Not too sure about this, because I also used it in the EFQ tests.
- Thought about adding a @todo to field_purge_data() or that function but I can't figure out what to write there. "Avoid passing partial entities around" doesn't make much sense to me. The point is that we don't have a choice right now, those entities might not exist anymore, we can't load them. Maybe we could mis-use entity lazy-loading (that would then fail if you try to load it) once we have it, not sure. That or actually introduce something like a EntityIdentifier class that implements EntityInterface as far as possible (that or we split it up) so that we have something we know can be identified as an entity but doesn't need to be fully loaded/loadable.
- The file_field_update() change was already committed in #985642: Remove file_attach_load() from file_field_update(), I just included it in the patch above to allow the tests to pass.
- Added a comment to TermStorageController, not extremely happy about the wording.
- The return value of EFQ::execute() now actually starts to make some sense to me. I changed it to exactly what is returned by the query and just made sure that bundle is defined. Which means that it's an object with the properties entity_id, bundle, revision_id and entity_type.
- BulkDeleteTest is full of weirdness, it's seriously hard to understand what's going on, but I'd prefer to not refactor too much in there, I guess we will have to re-visit that class anyway when merging test_entity (field.module almost-entity) with entity_test (real test entity from entity.module) in #1616930: Fix the field_test entity type
EfqTest and BulkDeleteTest's working locally, we'll find out about the rest soon.
Comment #22
BerdirAnd here's the interdiff against #12.
Comment #23
aspilicious CreditAttribution: aspilicious commented;)
-24 days to next Drupal core point release.
Comment #24
BerdirGrml, I actually noticed that as well before doing the commit but then managed to add a space so it doesn't count as a newline at the end of the file ;).
Re-rerolled. No other changes.
Comment #26
BerdirOk, this should fix the actual test fails, the variable table stuff must be some weird testbot hickup.
EntityPropertiesTest is another thing to be cleaned up, once the test_entity's have been moved to entity_test.module, that whole test class should be moved to entity.module and renamed, it has nothing to do with field.module nor entity properties.
Comment #27
aspilicious CreditAttribution: aspilicious commentedtypo
ITS GREEN! THANK YOU!
-25 days to next Drupal core point release.
Comment #28
BerdirWeird stuff removed.
Comment #29
BerdirComment #30
BerdirUpdating the values returned in field_test.module, this should be tested somewhere but I actually can't find it right now. Let's see if the tests pass.
Comment #31
chx CreditAttribution: chx commentedI 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.
Comment #32
fagothanks Berdir - the patch looks great! I've had a close look at it and found mostly nitpicks:
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.
with should be at the upper line
strange new line in there.
This adds an extra, unnecessary space to the indentation of the last line.
missing space after (object)
Comment #33
fagook, fixed the nitpicks and removed the taxonomy term creation change - let's see whether it's required.
Comment #35
BerdirIt fails because of the test that I added in the Taxonomy EfqTest class. While that assertion might look arbitrary on it's own, this is what field.module would do when that field_purge_data() stuff would be executed on a field attached to a taxonomy term.
We first noticed that problem in #1550454: Regression: Following taxonomy entity conversion, Taxonomy EFQ causes warnings and notices, where we solved it by removing the entity_create() from the entity_create_stub_entity() function as that resulted in way larger, classed objects than the simple stdClass objects in 7.x. Had quite a fight over that with @timplunket ;) However, we have to support it now for that specific case, it's no longer an issue in general because EntityFieldQuery doesn't call entity_create() by default, so we only get a Entity object when we really need it.
Comment #36
BerdirRe-roll with the term create changes re-added.
Comment #37
tim.plunketts/id/ID
Can we agree on a terminology? partial/incomplete sounds rather partial and incomplete :)
s/ids/IDs
Comment #38
BerdirImproved it a bit, as discussed in IRC, let's hope that code can be removed once vocabularies are configuration.
Comment #39
fagoI 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?
Comment #40
BerdirThe vocabularies as configuration issue (#1552396: Convert vocabularies into configuration) merges the vid and vocabulary_machine_name property into a single identifier (doesn't need a pk when it's configuration) that solves the problem. I also started a separate issue that implemented #1496612: Add bundle in the storage of taxonomy terms and it was closed in favor of the configuration issue by sun, I suggest you do the same with that one (comments is another thing, there it makes sense).
There is already a @todo there about moving it to Term::bundle(), I guess we would replace that, we can't do both? :)
Comment #41
fagoyep, 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.
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!
Comment #42
fagoComment #43
Berdirvid/vocabulary_machine_name: See #1551774: Replace taxonomy_term_data.vid with vocabulary_machine_name, especially the comments in comment #14 and later. The configuration patch over there should already contain the change (or did when I checked the last time) that keeps vid but makes it the machine name and therefore also changes vid to the machine name in taxonomy_term_data. Problem solved :)
And thanks for RTBC, yay!
Comment #44
webchick#38: stub-entity-concept-removed-1551140-38.patch queued for re-testing.
Comment #45
webchickCommitted 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.
Comment #46
tim.plunkettComment #47
BerdirHere we go: http://drupal.org/node/1721500
I wasn't sure if I should include the replacement function but I decided to not do that. I really do not want to see used. I know, I did myself in this patch and we should change that.
Comment #48
tim.plunkettLooks good to me. I agree there is no reason to advertise usage of a "private" (underscore-prefix) function.
Comment #49.0
(not verified) CreditAttribution: commentedUpdated issue summary.