entity_save() does not work with core node or user entities. They don't implement the save() method, and neither do their controller classes, apparently.

I can't imagine they're going to in drupal 7 either, so something else needs to be done to make entity_save useful until d8.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

naught101’s picture

Status: Active » Needs work
FileSize
1.13 KB

This is a hacky work-around.

naught101’s picture

On top of this, user_save() has a different format to other _save() functions in core, and would basically require, I think, the user to be loaded from the db, then diffed against the $entity, and then that diff would have to be parsed and passed back into $user_save() as the $edit array.

This might make http://drupal.org/node/981364#comment-3786118 very painful, since the "edit" is actually adding a field.

sun’s picture

I agree that entity_save() should special-case the entities of Drupal core and invoke ENTITY_save() accordingly.

naught101’s picture

fago’s picture

Status: Needs work » Closed (won't fix)

ehm, the the Entity CRUD API deals with Entity-CRUD-API-entities only. For dealing with others too, there is entity metadata, thus use entity_metadata_entity_save().

sun’s picture

Status: Closed (won't fix) » Needs review
FileSize
755 bytes

With due respect, entity_metadata module contains much more logic and behavior that is a very large overhead and completely needless for the task at hand. There's no need for UI handling, tokens, and whatnot.

It's unfortunate that we didn't manage to add entity_save() to D7 core. However, Entity API module should provide the CRUD operations for entities, taking core entities appropriately into account. entity_load() is handled by core already, so entity_save() is the missing and central counterpart to that.

fago’s picture

Status: Needs review » Closed (won't fix)

With due respect, entity_metadata module contains much more logic and behavior that is a very large overhead and completely needless for the task at hand. There's no need for UI handling, tokens, and whatnot.

Entity metadata doesn't do any UI handling or such, it's an API module - so it does what modules do with it. Indeed, there are tokens which I think about moving into a small separate module, for UX. see #975854: Move entity-metadata tokens into its own module..

Anyway, entity_metadata_entity_save() does what you want, and that in a way it actually works. I won't duplicate that in entity_save(), and I won't mix-up the two modules with two different purposes (see project page).

sun’s picture

How are modules supposed to figure out when to call entity_save() and when to call entity_metadata_save()?

fago’s picture

Component: Entity CRUD API - main » Documentation
Status: Closed (won't fix) » Active

Indeed, that might be confusing if one just looks up the function. We should improve the docs to clearly state the difference, what about adding a comment like that to each helper?

* This is a helper function for entity types provided by the entity CRUD API.
* In case you want to deal with any, arbitrary entity type make use of
* entity_metadata_entity_save() provided by the Entity metadata module.

sun’s picture

Sorry, that won't help much... You know, we have a serious problem here -- this issue and some others made it clear to me that this incarnation of entity.module is not what everyone (especially core developers) is expecting from "the" entity.module.

In speaking to others over the past weeks, everyone agreed that "the" primary entity.module needs to be a joint community effort with the goal of completing the entity system of Drupal core. It's totally not the intention to discredit your hard work, but so far, almost everyone who looked at the actual code of this incarnation immediately came up with the same conclusion -- it's not completing core APIs, but instead, building entirely new APIs around core APIs, introducing new concepts as well as new entity/API design patterns, etc.

So yeah, all of the additional ideas and functionalities in this current incarnation are nice and all, but it's definitely not what other module maintainers and developers actually need and expect. For that sake, various people are already considering to create a new entity.module that is actually doing what people expect it to do; i.e., completing core APIs and following existing design patterns in core, without introducing new ones unless actually required, and lastly, with the purpose of eventually moving that missing API functionality early into D8 core.

So far, I'm trying to prevent that from happening. However, given the confusion around very basic usage and integration issues as well as developer expectations around the current incarnation (for example, this very issue), as well as some of the replies heard/read here and there within this queue (I subscribed to all issues), it doesn't really look like this project has the intention to become the "joint-effort community consensus of Entity module", but rather understands the existing design and code as "set in stone".

If that is really the case, then I suspect and fear that we'll see a second entity.module very soon... Really, I would not like to see that to happen (not to mention that it's going to be hella confusing for developers as well as users)... but frankly, and sadly, as of now, I could perfectly understand that move. :-/

Overall, the situation with entity.module is very special and an entirely new challenge for the Drupal community. As far as I know, in Drupal's entire history, we never had the situation that the entire community already knows that a certain module is going to be a key building block for the entire eco-system. This is actually the case for Entity module, as its functionality is actually supposed to be in Drupal core, but merely is not, since we did not manage to tackle all entity subsystem features in the available time-frame. Therefore, it's mostly clear (at least for core developers) what functionality an Entity module is expected to contain and expose. But nevertheless, or perhaps exactly for that reason, the project/module requires a large community consensus and agreement, in order to be usable and adopted throughout contrib. [To amend that, some people already played with the idea of assigning Dries/webchick as maintainer of "the" supposed-to-be-in-core entity module for the same reason.]

Lastly, let me repeat that it's not my intention to insult you - most of the ideas and features in the current module are pretty darn cool! Also, I don't really know why no one else has spoken up in the queue so far, despite the partially heavy discussions that happened in IRC and elsewhere. At least I know that I'm probably one of the worst people to communicate stuff like this (language barriers)... but yeah, I finally wanted to reach out to you, so we can perhaps determine what we can do (or not). :)

Thoughts?

fago’s picture

hm, first off thanks for speaking up!

>In speaking to others over the past weeks, everyone agreed that "the" primary entity.module needs to be a joint community effort with the goal of completing the entity system of Drupal core.

I'd be very happy with the entity module being a joint community effort and I've always invited and encouraged every contribution, but I must say since the creation of the module over a year ago, there was not much input. Now with d7 nearing the release, people start to make use of it more and more, so the input increases; but well, now it's time for the module to settle and stabilize as well - in particular as it's a foundational module. That said, it's too late to re-architect the whole module at this point. To make this clear, I'm very much open to any changes and improvements to make things clearer and to bring the module up to that what the community would expect, but we have to consider BC, so it's just too late for any dramatical changes.

>it doesn't really look like this project has the intention to become the "joint-effort community consensus of Entity module", but rather understands the existing design and code as "set in stone".
As said, no nothing is set to stone. But modules are building upon this module and I won't scratch BC just for fun.

>it's not completing core APIs, but instead, building entirely new APIs around core APIs, introducing new concepts as well as new entity/API design patterns, etc.

I'd not say so and I don't see where the entity API fails to follow core in regard to this issue. We have no entity API in core handling saves, so entity_save() won't handle them. So for d7, there is and there must be a difference between handling saves via the provided CRUD API and between providing a compatibility layer in form of an unified save function on top of existing, diverse implementations. Mixing those up won't help anyone.
Regarding the bigger picture, yes the entity CRUD API extends the core API. It provides full CRUD as well as additional features which are all optional and might or might not be a good idea to do in d8. We'll see how they evolve during the d7 cycle.

If that is really the case, then I suspect and fear that we'll see a second entity.module very soon... Really, I would not like to see that to happen (not to mention that it's going to be hella confusing for developers as well as users)... but frankly, and sadly, as of now, I could perfectly understand that move. :-/

I really fail to see what another entity.module should do better and I'd invite anyone that is interested in working on an entity API to speak to me, before creating another one. That said, of course I can't prevent anyone from doing so.
So I'd love to see more contribution and/or co-maintainer(s) that help(s) making the project a success, but it's too late to start from scratch now.

klausi’s picture

The patch from #6 will not work for users, as the function signature and behavior of user_save() is different to for example node_save().

Form the project page:
entity.module: Simplifies creating new entity types.
entity_metadata.module: Provides a unified way to deal with entities and their properties.

So actually you want to use entity_metadata for unified entity access like with entity_metadata_save(). Maybe we have a naming problem here, maybe entity_metadata.module should be entity.module?

sun’s picture

We have no entity API in core handling saves, so entity_save() won't handle them. So for d7, there is and there must be a difference between handling saves via the provided CRUD API and between providing a compatibility layer in form of an unified save function on top of existing, diverse implementations.

That's not entirely true. While not officially formalized (again, due to time-restrictions), all of the core entities are implementing a consistent pattern:

ENTITY_load($id)
ENTITY_load_multiple(array $ids)
ENTITY_save($entity)
ENTITY_delete($id)
ENTITY_delete_multiple(array $ids)

whereas ENTITY == internal entity type name registered via hook_entity_info().

This doesn't imply that the idea of alternative ->save() and ->delete() controller class methods is wrong, but that's a new pattern, which - first and foremost - is inconsistent with the already established design pattern.

But yeah, disregarding the example at hand, we come back to the point that the current incarnation does not interpret semi-formalized patterns in Drupal core as something it should support, but rather offloads that entire core entity handling into an optional "metadata" module in order to register various custom entity info properties to formalize and introduce a new pattern.

the function signature and behavior of user_save() is different to for example node_save()

Yes, though we're still trying to resolve the main pain point for D7 over in #968458: Missing hook_entity_presave() -- but regardless of that, it's relatively easy to work around that issue. The question is whether each and every contributed module needs to implement that custom workaround code instead of calling entity_save() for users, or whether entity.module takes it over and properly handles it for everyone.

Right now, most code in entity.module is centered around "entity.module's entities", whereas everyone expects "the" entity.module to handle any kind of entity, regardless of whether it's a core entity, custom entity, or entity.module entity.

fago’s picture

>This doesn't imply that the idea of alternative ->save() and ->delete() controller class methods is wrong, but that's a new pattern, which - first and foremost - is inconsistent with the already established design pattern.

Right, as said - the CRUD API extends the existing pattern with CRUD. It's not inconsistent though, it's extending it.

The question is whether each and every contributed module needs to implement that custom workaround code instead of calling entity_save() for users, or whether entity.module takes it over and properly handles it for everyone.

Indeed, this is what is entity_metadata does for you.

Right now, most code in entity.module is centered around "entity.module's entities", whereas everyone expects "the" entity.module to handle any kind of entity, regardless of whether it's a core entity, custom entity, or entity.module entity.

No, all the code in entity.module is centered around "entity.module's entities" and everyone that has read its first sentence on the project page should be aware of that.

I guess this issue boils down to what klausi said above:

Maybe we have a naming problem here, maybe entity_metadata.module should be entity.module?

You want entity.module to do what entity_metadata does. But do you want it to also provide CRUD for new entity types? I know, that's it what people expect from the entity system for d8, and this is what entity.module does right now.

So if you suggest putting both into a single entity.module, I agree that it might look simple for devs at the first place - as yes, there would be just one entity_save(). However, I think it would just create more confusion in the end, as it would be completely confusing what is core, what core expects from an entity and entity.module, once for entity types implemented using its API, and once for others.

The reality is, we don't have a fully-unified CRUD system for entities in core (although we are close to), thus for being able to use CRUD we still need a slim abstraction layer and this is what entity metadata implements. It's not up to entity.module to mimic an "all-entities-have-CRUD" situation - we are not there yet, unfortunately.

fago’s picture

Under the light of the additional issue #986616: Update Manager fails when the primary module/theme for a project lives in a subdirectory I'm actually consider merging the modules.

Battle plan:
-> move entity.module to the module's root
-> move entity_metadata.module functions in a entity.metadata.inc, which gets included everytime
-> merge entity_metdata_entity_crud functions with their entity_crud respectives
-> to ease upgrading keep an empty entity_metadata module for BC, containing functions mapping to their new ones. Scratch that after at least one BETA release cycle.
-> #975854: Move entity-metadata tokens into its own module.

That should work, but we really need to be consequent in documenting which part is needed for what. Feedback appreciated.

naught101’s picture

From what you describe, that sounds like a great plan. Just to add two cents to sun's point, entity_load() already loads any entity, so from an outside perspective, it'd make send to have entity_save() and entity_load do the same thing. Maybe that's what you mean by merging the two module's CRUD functions...

fago’s picture

>Maybe that's what you mean by merging the two module's CRUD functions...
Yep, exactly.

fago’s picture

Title: entity_save() does not work with core nodes or users » merge both modules into one
sun’s picture

Title: merge both modules into one » Merge both modules into one
Component: Documentation » Entity CRUD API - main
Category: bug » task
Priority: Normal » Critical

Yes, the outlined plan in #15 makes sense. I'd consider this change as critical (especially since we're slowly reaching D7.0).

The only thing I'm not really comfortable with is the term "metadata", because it means all and nothing, and in particular, I fail to see the meaning of registering CRUD/callback information for entities. If entity.metadata.inc is going to register CRUD information for core entities, why not simply name it entity.core.inc?

bojanz’s picture

Interesting developments.

The whole point of entity metadata is providing information about entity properties (to contrib modules).
Doesn't sound logical to call it entity.core.inc.

naught101’s picture

Why are entity properties "metadata"? Aren't they just data? I don't see the distinction. Metadata would surely have to be defined on a per-entity type scale, or per bundle, and not in an API module?

bojanz’s picture

@naught101
Not properties, information about properties: title, description, datatype, getter callback (since the value might not be stored in the database, but calculated instead, the edit url for example), access callback how to sanitize it.... So I think "metadata" is the right word here.

And it is defined per entity type, by modules implementing hook_entity_property_info.
The metadata module currently does two things:
1) Provide metadata for core entities (which could be moved out)
2) Provide the basic wrapper functions (getter, setter, access callbacks, getting metadata, etc...) -> which I don't see where else could go...

fago’s picture

Exactly, it's info about entity properties, so metadata. Anyway, I think the term will hide more with the merge.

>CRUD/callbacks.
As when we merge the usual CRUD callbacks into entity_save() which resides in the main .module file, it probably doesn't make much sense to move it into an include anyway.

So what about an entity.property.inc include file instead then - which only contains helper/utility functions around the introduced hook_entity_property_info()?

The hook implementations for the core modules reside in the modules/* includes anyway. However there is also modules/callbacks.inc - which contains various callbacks for CRUD or property info. Suggestions for a better name for it are welcome.. !

sun’s picture

So if all of this meta data is about entity info, then the proper name for an include file (if any) would be entity.info.inc. More or less in line with Field module's field.info.inc (even though that it doesn't sound too similar).

fago’s picture

@entity.info.inc - yes, it already uses that include files for providing property info. I've used entity.property.inc though for providing API functions around that, e.g. to get all defined property info.

In the meanwhile I've started working on that big-bang. You can find the changes in the git repository over there: https://github.com/fago/entity/tree/988780 (I guess a patch won't help anyone as quite some stuff got moved around).

Status:
I've moved everything around and create the interim-functions to ease upgrading. Also upgrading the module is rather weird, though I managed to get it working. For that it was required to leave entity.db.inc in place for the upgrade. Without that file even update.php cannot be accessed as modules might require the file to be loaded, thus the bootstrap requires it. Once people have upgraded, that can be removed - so we can do so in a subsequent release.

Todo:
* Merge the API docs
* Fix README.
* Update the provided core property info to make use of new callback names

Any comments?

Note: This also fixes #989670: Updating with the Update-manager fails and #975854: Move entity-metadata tokens into its own module..

amitaibu’s picture

Subscribe. OG will also need an option to $entity->save() where $entity might be core entity Or entity API entity.

fago’s picture

Status: Active » Needs review

ad #26: You can already use entity_metadata_entity_save() for that, but once this is fixed it will be just entity_save().

ad #25:
I've implemented the missing points. See https://github.com/fago/entity/tree/988780. Comments?

fago’s picture

Status: Needs review » Fixed

With the d7 release nearing, it's really time to move on and fix that. So I've gone ahead and committed this one. Let's incorporate any feedback with follow-up issues.

Summary of changes:

The "entity metadata" and the "entity" module have been merged into a single entity.module, which now lives in the main module directory. Also, providing token replacements has been moved into a small new module "entity tokens", such that people can easily control whether they want to get the tokens.

API changes:

  • entity_metadata is gone. Remove any dependencies on it and just depend on 'entity'.
  • entity_metadata_entity_* (save, create, delete, access, view, ..) functions have been deprecated by just using the entity_* functions, which now deal with all entity types.
  • Also a few other functions related to entity properties and previously provided by entity_metadata have been renamed, e.g. entity_metadata_get_info() to entity_get_property_info().
  • The callbacks provided for hook_entity_property_info() have been moved from the entity_metadata_* namespace to the entity_property_* namespace, thus deprecating the old callbacks. In particular, entity_metadata_verbatim_set() is now entity_property_verbatim_set(), just as entity_metadata_verbatim_get() is now entity_property_verbatim_get(). Functions for backward compatibility are provided, but it is suggested to make use of the new callback functions now.

For now there is an "entity_metadata" module that provides backward compatibility for all function-name changes. It contains a function for each re-named function, which points to the new one. This module is there to ease upgrading modules and will be removed in the near future, e.g. after one beta-release period.

drunken monkey’s picture

Status: Fixed » Needs work
FileSize
5.11 KB

There are still a ton of outdated references to entity_metadata function in the entity module. The attached patch is the minimum to let Search API work without immediate fatal errors if entity_metadata.module is disabled.
But I'd bet there are several others as well.

sun’s picture

To resolve the WSOD after updating to latest Entity module code, the core patch in #996236: drupal_flush_all_caches() does not clear entity info cache was required.

fago’s picture

ad #29:
oh, thanks. It looks like the BC module was included while running the tests, although the test doesn't setup it. I've worked around that and rolled a new patch which makes the entity-metadata tests work without the BC module. Patch attached.

ad #30:
Strange, as noted there updating did work for me during my test-runs. Have you tried updating the entity API alone or did you also change your custom controller class?

fago’s picture

Status: Needs work » Needs review
FileSize
13.7 KB
fago’s picture

Status: Needs review » Fixed

Committed this follow-up fixes.

fago’s picture

I've just released beta4 containing this changes + updated the project page to reflect this.

naught101’s picture

awesome. thank you :)

Status: Fixed » Closed (fixed)

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