Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
8.47 KB

First attempt at a patch.

The current entity thing is broken because the entity type relies on the fact that entity_load() by default contains a static cache and does it's caching through that. However, files aren't using that, so it always re-loads the file from the database. So I addded a reference to keep the loaded entity.

That needs to be cleared when fid changes, did that by re-routing the __set() call through the entity instead of the fid property. Also the code in FileItem is ugly and should possibly be merged together with the taxonomy reference item, from where I copied it? It's quite a bit of code...

Also, I guess the taxonomy tests will fail now as they will require the same change.

Status: Needs review » Needs work

The last submitted patch, file-item-field-1839068-1.patch, failed testing.

fago’s picture

Issue tags: +Entity Field API

tagging

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.96 KB
6.48 KB

Discussed with @fago, we agreed on not supporting reference changes like that officially, so removed test coverage and the ugly code to make it work.

Status: Needs review » Needs work

The last submitted patch, file-item-field-1839068-3.patch, failed testing.

plach’s picture

Issue tags: +node ng
Berdir’s picture

Ok, I think because this does not work correctly when using the BC decorator and converted field types:

$grouped_items[$field_id][$langcode][$id] = isset($entity->{$field_name}[$langcode]) ? $entity->{$field_name}[$langcode] : array();

in _field_invoke_multiple().

It looks like isset() returns FALSE for comment_body and the file field there I'm positive that there are values :)

fago’s picture

Status: Needs work » Needs review
FileSize
2.07 KB
8.55 KB

ok, I had a look at it and found some problems, see attached interdiff.

- file_download access implementation was using field_get_items() without enabling BC-mode. I added BC-mode directly to field API helper.
- Some $item was casted to $file and assumed that looks like an entity. But our $item is clean, so there will be no other properties like the expected 'uri' in there. Fixed this by doing a proper entity_load() - not ideal as file entities miss a static caching currently, but sure this is what happens when it is disabled ;-). Not sure why it's disabled?
- There was a $comment-field usage which was not yet converted to NG. It worked previsouly as the field wasn't registered as proper field, but now with adding support for file fields it is.

Berdir’s picture

Thanks!

- Some $item was casted to $file and assumed that looks like an entity.
But our $item is clean, so there will be no other properties like the
expected 'uri' in there. Fixed this by doing a proper entity_load() -
not ideal as file entities miss a static caching currently, but sure
this is what happens when it is disabled ;-). Not sure why it's
disabled?

About the first part, I already tried to change that when converting file to a classed entity. The problem is that we need both, the properties from $item (notably, the description) and the file entity properties. So a file_load() there *should* result in test failures. If not, then we are missing test coverage :)

Might be relatively easy to fix by introducing a separate, optional theme function argument for the description? I think that's the only thing that is needed there, although images will probably have similar issues.

Berdir’s picture

Ok. No test fails so that means we have zero test coverage for the description :)

This patch updates FileFieldDisplayTest to include a test for the description. Also fixed the completely borked test while doing that:

The test looped through all field formatters and hidden was the last. But that was never reverted so all tests below were executed with the hidden test formatter. The positive test worked around that by testing the edit page and all "does not display X" tests were completely bogus.

Note that we still have a problem.

But our $item is clean, so there will be no other properties like the
expected 'uri' in there

So item being "clean" is a problem. Previously, we were able to load all file fields at once, when displaying 10 nodes with 10 files each in a list, there was a single file_load_multiple() to load them all. Now the prepare view code is useless unless we enable static caching for files and just do the load there to fill the static cache. Right now, we load all files *twice*.

But there are other problems related to this that we will run into. For example, text_field_load() is currently completely broken if I understand this correctly.

fago’s picture

Might be relatively easy to fix by introducing a separate, optional theme function argument for the description? I think that's the only thing that is needed there, although images will probably have similar issues.

This confuses me a bit. I'd assume #file should be a reference on the file entity, so why should be the description in there? Assuming it should be a merged structure is weird. The patch separating #description makes sense.

Now the prepare view code is useless unless we enable static caching for files and just do the load there to fill the static cache. Right now, we load all files *twice*.

Right. We should think about how we approach this kind of multiple-loading cases, in particular if static caching is disabled. We could have a helper that multiple-loads on the field class level or just take care of it in pre-render, but we need to be careful to avoid introducing any stale caches....

But there are other problems related to this that we will run into. For example, text_field_load() is currently completely broken if I understand this correctly.

Yep, if we have static caching for things like that I think it should go into the getValue() method of the respective class.

Berdir’s picture

This confuses me a bit. I'd assume #file should be a reference on the file entity, so why should be the description in there? Assuming it should be a merged structure is weird. The patch separating #description makes sense.

It *is* confusing :)

This basically dates back to when file_field_load() loaded the file and merged everything into $item for caching*. $item and $file was treated as basically the same thing and was happily converted back and forth with (object) and (array). The main difference was that $item additionally contained a description key and that's why that theme function currently checks it like that.

I'm worried about field and entity caching currently:
* We're back to having huge, recursive entity objects (ever tried debug($comment) ?) with a lot of meta information in them. I have no clue what is going to happen if you try to enable persistent caching. If we just serialize the entities then we might end up moving and serializing so much data back and forth that loading for the database is faster ;)
* The main point of hook_field_prepare_view() is to check/load/calculate stuff only when it's viewed but in a more efficient way than for every hook_field_formatter_view() invocation separately. I think we need to find a way to keep the usefulness of that hook or replace it with something comparable without having to rely on the static cache of the entity system. One way to do that would be to make ->entity a mix of a calculated and statically persistent property. Meaning, if you access it and it's not set then we do what we do right now. return entity_load(). However, you can do ->entity = $entity and then that value is kept. That would be close to what prepare_view() currently does.
* hook_field_load() is similar but as you said, we should be able to replace this with calculcated properties in setValues(). That does however require two things: a) we actually need to define them. right now, I think text_field_load() is broken for NG. b) Support (field) caching for calculcated properties.

Also asked @davereid to have a look at this issue and the file changes.

*That is likely also the reason why files have static caching disabled. They're mostly used for file/image fields and back then, that would result in statically caching them twice in most cases.

Dave Reid’s picture

fago’s picture

* We're back to having huge, recursive entity objects (ever tried debug($comment) ?) with a lot of meta information in them. I have no clue what is going to happen if you try to enable persistent caching. If we just serialize the entities then we might end up moving and serializing so much data back and forth that loading for the database is faster ;)

That would be good to back up by actual numbers? :) I doubt it's that bad.
debug($comment) blowing up sucks, yep. I'd have preferred to avoid recursive references as well, but it looks they are necessary to keep the code reasonable.

Meaning, if you access it and it's not set then we do what we do right now. return entity_load(). However, you can do ->entity = $entity and then that value is kept. That would be close to what prepare_view() currently does.

Yes, I think it's main purpose is leveraging multiple loads. I'd be happy to discuss the proposed idea (in its own issue), but it would have to multiple load also for being able to serve as prepare_view() replacement.

* hook_field_load() is similar but as you said, we should be able to replace this with calculcated properties in setValues(). That does however require two things: a) we actually need to define them. right now, I think text_field_load() is broken for NG. b) Support (field) caching for calculcated properties.

text_field_load() is just ignored by NG, yes.

Support (field) caching for calculcated properties.

That should be easily controllable by the computed property once we serialze the NG-objects for caching. The class then has full control on whether it's statically caching and also on whether it keeps the cache during serialization.

So should we open a separate issue for flushing this out?

fago’s picture

Status: Needs review » Needs work
+++ b/core/modules/file/lib/Drupal/file/Type/FileItem.php
@@ -0,0 +1,93 @@
+   * @see self::getPropertyDefinitions()

Should refer to the class, see others.

Except from that, the patch looks already good + brings test coverage for the description. Good work!

So item being "clean" is a problem. Previously, we were able to load all file fields at once, when displaying 10 nodes with 10 files each in a list, there was a single file_load_multiple() to load them all. Now the prepare view code is useless unless we enable static caching for files and just do the load there to fill the static cache. Right now, we load all files *twice*.

Right, so what about
a) open an issue for flushing out the caching strategy in general
b) do #1292374: Enable static caching for File entities
c) move on with this?

Berdir’s picture

Status: Needs work » Needs review
FileSize
509 bytes
12.21 KB

That abc sounds good to me. It's also easy to fix text_field_load() (if we actually want to) by simply declaring those properties. I did that for the test field in the test_entity/entity_test issue and it works just fine then.

Re-roll to fix the self::.

fago’s picture

Status: Reviewed & tested by the community » Needs review

Good, setting to RTBC then!

fago’s picture

Status: Needs review » Reviewed & tested by the community

Status: Needs review » Needs work

The last submitted patch, file-item-field-1839068-16.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.74 KB
12.46 KB

'entity type' is now 'EntityType' ;)

Fixed the tests and converted them to the unit test base class.

Status: Needs review » Needs work
Issue tags: -Entity Field API, -node ng

The last submitted patch, file-item-field-1839068-20.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Entity Field API, +node ng

#20: file-item-field-1839068-20.patch queued for re-testing.

fago’s picture

Status: Needs review » Reviewed & tested by the community

I see. Great to see it using unit tests as well now, so let's move with it now?
-> RTBC

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/field/field.moduleundefined
@@ -996,6 +996,7 @@ function field_view_field(EntityInterface $entity, $field_name, $display_options
 function field_get_items(EntityInterface $entity, $field_name, $langcode = NULL) {

This could use a @todo.

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/formatter/GenericFileFormatter.phpundefined
@@ -35,7 +35,8 @@ public function viewElements(EntityInterface $entity, $langcode, array $items) {
-        '#file' => (object) $item,
+        '#file' => file_load($item['fid']),
+        '#description' => $item['description'],

Is the file already loaded by this point? If not that might need to be loaded with file_load_multiple() in a prepare_view() hook rather than here. Needs profiling unless I'm missing something.

Berdir’s picture

Yes, we have a prepare_view() that loads all files. files now use the static cache, so that file_load() is guaranteed to go against the static cache. See discussion starting in #10.

Not sure what @todo you want to see? I seems unecessary to me to add a @todo to every single place where we use the BC decorator, the whole thing has to go away again and then have no choice but to change that again anyway. In fact, I'm wondering if we still need field_get_items() at all or if we can replace that with the methods/magic stuff on EntityNG.

plach’s picture

In fact, I'm wondering if we still need field_get_items() at all or if we can replace that with the methods/magic stuff on EntityNG.

AFAIK we are not applying language fallback rules anywhere right now, which field_get_items() does.

Berdir’s picture

Status: Needs work » Needs review

#20: file-item-field-1839068-20.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Requested a re-test and back to RTBC as per @catch in IRC.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Entity Field API, -node ng

The last submitted patch, file-item-field-1839068-20.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Entity Field API, +node ng

#20: file-item-field-1839068-20.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

That was a random failure, back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x.

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