Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Comment | File | Size | Author |
---|---|---|---|
#36 | image-field-type-1839066-36.patch | 8.69 KB | Berdir |
#36 | image-field-type-1839066-36-interdiff.txt | 861 bytes | Berdir |
#31 | image-field-type-1839066-31.patch | 8.51 KB | Berdir |
#29 | image-field-type-1839066-29.patch | 10.98 KB | Berdir |
#29 | image-field-type-1839066-29-interdiff.txt | 760 bytes | Berdir |
Comments
Comment #1
plachComment #2
BerdirHere's a first patch for this.
Seems to work fine, let's see if there are any test fails.
Not happy about the test files that I used, I need real images and drupalGetTestFiles() is not available in DUBT, which is something that we might want to change?
Comment #3
das-peter CreditAttribution: das-peter commentedwith what? :)
Would be "// Create a test entity with the image field set." okay?
Attached patch only adds
image field set.
Is this behaviour intended? Shouldn't it also be triggered by changing the fid?
Could we unset the width automatically if a different fid is set in
ImageItem::setValue()
?Comment #4
BerdirOpened #1919088: width/height of image field not updated when fid changes for the width problem as it wasn't introduced here, it simply has never been tested like this before. Not 100% sure what's the best way to detect and re-act to that, so I think doing it in a follow-up makes sense.
Your comment change looks good to me, I think it's fine if you RTBC despite making such a small change.
Comment #5
plachLet's do this.
Comment #6
fagoCode looks good and I agree that it makes sense to work on the height issue in a follow-up
I found the following minor doc issues, again that can be fixed as follow-up also.
This should be @file ;-)
again @file
Yeah, the references are being reworked at #1868004: Improve the TypedData API usage of EntityNG. We need to figure out on how to deal with updates best, e.g. maybe we should make it possible for the parent to get notified when the child changes... I guess this would be useful in many cases, e.g. also if we want to track which entity fields change.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedCouldn't ImageItem extend FileItem? There seems to be heavy overlap between them.
Comment #8
BerdirHm. All *Item class look similar.
I'd prefer to get it in first and then look if we can unify them afterwards I don't think we can actually save much code at the moment here. They have the same two methods, yes but those are different as they are defining and checking different properties.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedFor me, it's not so much about code reduction as about the Serializer's Normalizer having a way to know that two field item types have some kind of relationship.
Comment #10
amateescu CreditAttribution: amateescu commentedActually, there is a lot of code that can be saved if we extend
FileItem
and I think it makes a lot of sense to do so in this patch.Comment #12
Berdir#10: image-field-type-1839066-10.patch queued for re-testing.
Comment #14
amateescu CreditAttribution: amateescu commentedI wonder if @plach can shed some light on that failing test. I've tested #3 locally and it has the same fail, so the problem is not with the new patch :/
Comment #15
amateescu CreditAttribution: amateescu commented#3: image-field-type-1839066-3.patch queued for re-testing.
Asked for a re-test on #3, just to be sure I'm not crazy.
Comment #16
fagoI'd be happy to see this extending the FileItem class, but #10 demonstrates that the sub-type relationship is not given. We do not have all properties of FileItem in the ImageItem, so it's not a candidate for the sub-type relationship. We can make it so though, but I agree that we should move this to its own issue.
I take a look on whether I can find the source of the test fails.
Comment #17
fagook, looks like we have multiple problems here.
1) We create a new translation without providing a value for a translatable property, i.e. "name" of the test entity. This would have to save as NULL, but our schema says that's not valid. Thus, we need to fix our schema here. I've attached a short-gap fix for that in the updated patch.
I think we should generally take care of our schema definitions and make sure they match the API. I've commented on that here: http://drupal.org/node/1777956#comment-7156374
2) It looks like the BCEntity decorator results in a new "und" translation being created, this is why tests still fail. The storage controller thinks there should be a "und" translation, while there isn't. Thus, we need to make sure the BCEntity takes care of removing un-used und values afterwards or take this into account when calculating availableTranslationLanguages().
Comment #19
BerdirRe-roll of #17 without the entity_test.install change. I confirmed that this passed with the EntityBCDecorator improvements from #1818556: Convert nodes to the new Entity Field API
Comment #21
fagoLooks like the hunk from #17 is still required? (what makes sense)
Comment #22
BerdirNo, this is supposed to fail. It should pass once the Node NG patch is in.
Comment #23
fagoI see. If it passes with a non-NULL name, that's a bug though. But it's not related to this, so if not required here it should become its own issue.
Comment #24
Berdir#19: image-field-type-1839066-19.patch queued for re-testing.
Comment #26
Berdir#17: d8_image.patch queued for re-testing.
Comment #27
BerdirRe-roll of #17, das-peter said that it is working for him but it fails for me. Let's try again.
I got confused and tested the wrong test before, the EntityBCDecorator changes don't fix it for me, at least.
Comment #29
BerdirWeird. I don't understand why those values are only set in one single case but excluding them from the list makes it work.
@amateesu also said that he can't reproduce the test failures that I have on php 5.4, so it's not that.
Comment #30
jibranI think this got mix up in merge. #1934716: [Followup] Merge system.fast_404 config object into system.performance
Comment #31
BerdirYep, re-rolled.
Comment #32
fagoLooks good.
I think this is ready and the last issue needed for #1732730: [meta] Implement the new entity field API for all field types. Reviews/RTBC anyone?
Comment #33
plachOverall looks good to me, I have two minor complaints and a question:
Search/replace leftover? ;)
Why does this not need to be declared as an entity reference?
If I'm not mistaken this is not covered by the test.
Comment #34
Dave ReidIs this where we should be enforcing that height and width properties should not actually be editable, and they're in-fact derivative from changing the fid value?
Comment #35
BerdirBasically yes, also automatically updating it when the file changes. See the @todo in the test. However, I'd like to do that in a follow-up issue, depends on some Entity Field API issues that allow the parent (e.g. the file item) to properly re-act to changes in it's properties.
Comment #36
BerdirRe-roll, fixed the @image and added tests for that part.
fid is not an entity reference because it's on the same level, it has fid and entity properties like an entity reference has target_id and entity. It's a different kind of entity reference, one that is specific to files. It probably should become an entity reference at some point or extend it but that would also imply to change all access to it (target_id, not fid) and the storage.
Comment #37
plachCool
Edit: Actually I missed the following:
Comment not wrapping correctly.
I guess we can fix this on commit or if this needs a reroll.
Comment #38
Berdir#36: image-field-type-1839066-36.patch queued for re-testing.
Comment #39
catchCommitted/pushed to 8.x, thanks!
Comment #40
plachComment #41
plachwtf