Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#20 | file-item-field-1839068-20.patch | 12.46 KB | Berdir |
#20 | file-item-field-1839068-20-interdiff.txt | 2.74 KB | Berdir |
#16 | file-item-field-1839068-16.patch | 12.21 KB | Berdir |
#16 | file-item-field-1839068-16-interdiff.txt | 509 bytes | Berdir |
#10 | file-item-field-1839068-10-test-only.patch | 10.37 KB | Berdir |
Comments
Comment #1
BerdirFirst 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.
Comment #3
fagotagging
Comment #4
BerdirDiscussed with @fago, we agreed on not supporting reference changes like that officially, so removed test coverage and the ugly code to make it work.
Comment #6
plachComment #7
BerdirOk, I think because this does not work correctly when using the BC decorator and converted field types:
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 :)
Comment #8
fagook, 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.
Comment #9
BerdirThanks!
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.
Comment #10
BerdirOk. 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.
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.
Comment #11
fagoThis 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.
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....
Yep, if we have static caching for things like that I think it should go into the getValue() method of the respective class.
Comment #12
BerdirIt *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.
Comment #13
Dave ReidRelated:
#1292374: Enable static caching for File entities
#1448124: Image dimensions should be available from file_load() for images, and not stored in field data
Comment #14
fagoThat 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.
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.
text_field_load() is just ignored by NG, yes.
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?
Comment #15
fagoShould refer to the class, see others.
Except from that, the patch looks already good + brings test coverage for the description. Good work!
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?
Comment #16
BerdirThat 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::.
Comment #17
fagoGood, setting to RTBC then!
Comment #18
fagoComment #20
Berdir'entity type' is now 'EntityType' ;)
Fixed the tests and converted them to the unit test base class.
Comment #22
Berdir#20: file-item-field-1839068-20.patch queued for re-testing.
Comment #23
fagoI see. Great to see it using unit tests as well now, so let's move with it now?
-> RTBC
Comment #24
catchThis could use a @todo.
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.
Comment #25
BerdirYes, 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.
Comment #26
plachAFAIK we are not applying language fallback rules anywhere right now, which
field_get_items()
does.Comment #27
Berdir#20: file-item-field-1839068-20.patch queued for re-testing.
Comment #28
BerdirRequested a re-test and back to RTBC as per @catch in IRC.
Comment #30
Berdir#20: file-item-field-1839068-20.patch queued for re-testing.
Comment #31
BerdirThat was a random failure, back to RTBC.
Comment #32
catchCommitted/pushed to 8.x.