Posted by plach on February 17, 2013 at 1:46am
10 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | entity system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | API clean-up |
Issue Summary
Right now entity_load_unchanged() simply wraps a cache clear + entity load. In OO code it's not recommended to call procedural functions which live in the global scope, hence we need to inject the entity manager and retrieve the proper storage controller. Since the load unchanged logic lives only in the function above, OO code needs to recode it every time. Although the implemented logic is quite simple this in not exactly DRY.
See #1807692-82: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget and below.
Comments
#1
Here is a patch.
#2
Looks fine to me.
Just one thing: Can we make the method body in ConfigStorageController identical to DatabaseStorageController, please? The resetCache method exists, and is merely empty right now, but we want to change that in #1885830: Enable static caching for config entities
#3
Here it is.
Btw, would it make sense to have an abstract
BaseStorageControlleras a common ancestry between database and config storage controllers to avoid repeated code?#4
Thanks!
Yeah, I totally thought of a EntityStorageControllerBase a couple of times already, too. We might want to do that as part of #1893772: Move entity-type specific storage logic into entity classes.
#5
Yes, I was thinking about that one too :)
#6
+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerInterface.phpundefined@@ -41,6 +41,17 @@ public function resetCache(array $ids = NULL);
+ * @param $id
+ * The ID of the entity to load.
+ *
+ * @return
+ * The unchanged entity, or FALSE if the entity cannot be loaded.
+ */
+ public function loadUnchanged($id);
Nitpick, but the @param and @return should specify the type.
#7
There is no fixed type for the entity ID, right?
(no interdiff I just edited the patch :)
#8
Thanks.
#9
Hmm while we're moving this could we rename it to loadUncached() or similar?
#10
Not sure about this: currently the method name describes what you'll get, if we rename it we are exposing how we provide the unchanged stuff, which may need to be updated if we change the underlying implementation.
#11
Hmm yeah I'm not sure either. This was originally added for hook_node_*() implementations that want to see the original entity before it was loaded rather than the one as it's being changed, but we now have $entity->original (which is messy in itself but for the moment that's what we have).
The only place this is used outside of storage controllers and tests is http://api.drupal.org/api/drupal/core%21modules%21translation_entity%21l...
But could that use $entity->original instead?
If it can, then we could probably refactor the tests a bit and make this a protected method.
#12
If we make
$entity->originala proper and reliable entity property then the field synchronizer can totally leverage it. Then we can make the method protected.#13
@catch:
Do you think we should do that in this issue ot it can be another one? I've the feeling it'll be easier to do once the NG migration is complete.
#14
Separate issue seems fine but could we create that now and add a @todo?
#15
I think that loadUnchanged() is actually much more appropriate than loadUncached().
The focus of this facility is not on the cache, because loading the [database-]cached copy of the entity would be totally OK. loadUnchanged() actually works around the static cache, because the static cache is compromised with edited, in-process, submitted form values.
#16
Here is the todo.
Actually I'm thinking that perhaps it would be better to add a method to
EntityInterfaceto retrieve the unchanged entity as a replacement to$entiy->originalinstead of defining a property. This would be a more solid solution and wouldn't require us to wait for the NG conversion to be completed. If we decied to go this way we can hijack this issue and skip the follow-up.#17
I'm not sure about #16, and likewise, also not sure about the added @todo.
I could foresee situations in which you want to populate $entity->original with a fake/dummy entity that isn't actually loaded from storage. But turning ->original into a method would enforce storage-loading on all possible scenarios.
In any case, this particular and focused change looks good to me. Let's handle any further/additional questions in separate issues instead.
#18
Makes sense, here is the follow-up: #1925914: Make $entity->original a defined entity field.
I introduced the Post NG clean-up tag, since I have lots of issues/todos pending on that.
#19
#20
Good clean-up! I found a nit-pick:
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php@@ -143,6 +143,15 @@ public function load(array $ids = NULL) {
+ function loadUnchanged($id) {
"public function"
+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php@@ -236,6 +236,15 @@ public function load(array $ids = NULL) {
+ function loadUnchanged($id) {
should be "public function"
#21
Here it is.
#22
Not necessarily, we could add a setter as well. If we had it as a method, we'd only need to load the entity when accessed, right now it's wasted effort when it's not needed.
#23
Thanks plach, back to RTBC then.
Ideally, we'd have the entity itself tracking what has changed imo - so we could save the load operation at whole. Howsoever that discussion belongs into its own issue.
#24
Yes that's an even better point. I thought we had an issue for this already, but it may be stuck in one of the very early entity API dicsussions on Drupal.org. At the risk of a duplicate issue opened #1926714: Track changes for entity property changes and make available to hooks.
#25
Committed/pushed to 8.x, thanks!
Will need a change notice.
#26
Here it is: http://drupal.org/node/1935744
#27
Small follow-up: #1935762: Remove entity_load_unchanged() call from FieldTranslationSynchronizer::synchronizeFields().
Nothing special in itself, but since it's probably setting a standard it could use some serious DX review. Quoting @yched from #1807692-82: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget:
#28
The change record looks good. I searched through d.o including examples module, where this change could be reflected but did not find anything. If no objections, I change the status to fixed.
Good job, plach!
#29
Automatically closed -- issue fixed for 2 weeks with no activity.