Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Now as the new entity field API got committed, we need to convert existing entity types to make use of it. See #1346214: [meta] Unified Entity Field API and the "Entity Field API" tag.
Comment | File | Size | Author |
---|---|---|---|
#68 | file-entity-ng-1818568-67-interdiff.txt | 1.8 KB | Berdir |
#67 | file-entity-ng-1818568-67.patch | 184.65 KB | Berdir |
#67 | file-entity-ng-1818568-67-interdiff.txt | 1.8 KB | Berdir |
#65 | file-entity-ng-1818568-65.patch | 184.2 KB | Berdir |
#63 | file-entity-ng-1818568-63.patch | 184.12 KB | Berdir |
Comments
Comment #1
fagoadding tag
Comment #2
Gábor HojtsyJust noting this is not a priority for the multilingual initiative because files themselves contain no human facing textual component. The file descriptions are in the file fields, which are already translatable.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedI should note that this is a priority for REST, though. We want to support RESTfully creating file entities (like images for image fields).
Comment #4
Crell CreditAttribution: Crell commentedTagging per #3: #1927648: Allow creation of file entities from binary data via REST requests
Comment #5
BerdirHere's a first patch. Attempting a full conversion without BC decorator.
Lots of tests passing already.
One problem is that we still have $file objects that aren't entities, mostly in test (drupalGetTestFiles()), so that's a bit confusing sometimes. There's also some file entity specific code left in file.inc that should be moved to file.module.
Also, field items are messy, as preloading doesn't really work as it did before. Default images for example are currently broken.
Comment #7
BerdirPatch didn't apply anymore.
Comment #9
BerdirFixing some tests. Ugly stuff in locale.module :(
Comment #11
BerdirMore test fixes. Most except the locale stuff should be fixed.
Comment #13
Berdir#11: file-entity-ng-1818568-11.patch queued for re-testing.
Comment #15
BerdirForgot the use there.
Comment #17
BerdirOk, let's do this the other way round. Instead of enforcing $file to be a file entity in locale stuff, I'm enforcing stdClass now and convert the few cases that are file entities (manual uploads) to stdClass. There are existing issues to convert that stuff to separate classes, they're not really file entities anyway, they simply overlapped enough so that it worked.
Comment #18
BerdirComment #20
Berdir-16 was the interdiff. Not sure what I was thinking there..
Comment #21
BerdirThis might be green.
Comment #23
das-peter CreditAttribution: das-peter commentedI don't think we should compare whole objects in the
FileFieldRevisionTest
when it's just about the fid.I've changed the related assertions accordingly and locally it passes.
Comment #24
BerdirRe-roll, no other changes yet.
Comment #26
das-peter CreditAttribution: das-peter commentedFixed the re-roll, there were some deprecated constants leftover.
Comment #28
das-peter CreditAttribution: das-peter commentedWe've to reload the file, otherwise the new path isn't set. It's not a reference.
And
FileFieldRevisionTest
shouldn't compare the whole file entity objects but their identifiers.Patch changed accordingly.
Comment #29
BerdirAs we have to change all lines anyway, I decided to start adding methods.
- Only added getter's for now, not sure about adding setters as we only have very few use cases or using method names that allow get and set at once.
- Replaced all occurrences that I could find, might have one or two wrong ones, the tests will tell.
- Have not yet added one for the uid, as there's targetId and entity, not sure what to do. GetUid()/getUser(), getUser/getUserId()?
There are also many cases where we have repeated patterns, e.g. format_size() of the filesize, checking if the file exists. So we could add more methods but that's for a follow-up.
Comment #30
BerdirYeah, a few ones are wrong. This should be better. Not sure if field->value = $something or field = $something is better, that's mixed up a bit...
Comment #32
Berdir#30: file-entity-ng-1818568-30.patch queued for re-testing.
Comment #34
BerdirRe-roll.
Comment #36
Berdir#34: file-entity-ng-1818568-34.patch queued for re-testing.
Comment #38
msonnabaum CreditAttribution: msonnabaum commentedGreat work. It's nice to see FilesInterface get some methods :)
Since this is already the File entity, I dont see a reason to repeat the word "File" in method names like that. I think they all work better with it removed.
I'd probably name that something like getCreatedTime(), getCreatedTimestamp(), or maybe even createdAt(), to better reflect what that property returns.
Is there a use case for getting the id instead of just always returning the entity? The latter makes more sense I think if we can get away with it.
getUser() is ok, but it'd be nicer if we could also reflect the association better, like getOwner() or getAuthor().
Comment #39
BerdirThanks.
- Renamed getFileMime() and getFileSize(), kept getFileUri() as discussed to separate it from uri().
- Added getOwner(), that matches nicely with the token format that we already have: [file:owner:uid]
- Renamed getTimestamp() to getCreatedTime()
- Not sure yet about methods for status... files can be temporary or permanent, should we add isTemporary()/isPermanent()? How do you make something temporary/permanent? setPermanent()? permanent()? permanize()? :p
- Also fixed the failing test, let's see if I broke something.
Comment #41
BerdirAnd this should fix the last test failure there.
Comment #42
moshe weitzman CreditAttribution: moshe weitzman commentedsetPermanent() sounds like a good method name to me.
Comment #43
msonnabaum CreditAttribution: msonnabaum commentedTemp almost feels like a different object, but I think isPermanent/isTemporary and setPermanent/setTemporary are maybe the best we can do with methods.
Comment #44
BerdirSure thing. Looks nice I think.
I think them being the same thing makes sense, uploaded files are always temporary first and are made permanent as you add usage of the file. It would be weird if it would suddenly become something else, no?
There are a few places that depend on the file entity/interface that are not yet in file.module. file_save_upload() in file.inc, file tokens in system.module, some assertion methods in FileTestBase. I suggest we move them in a follow-up, even though I started to document the @return and @param's properly so that it's easier to review the changes.
Comment #46
BerdirStupid mistakes are stupid.
Comment #48
BerdirThis should be green again.
Comment #49
BerdirJust noticed that timestamp is actually the changed timestamp, so the method name isn't correct.
Comment #50
BerdirChanged that to getChangedTime().
Comment #52
BerdirFixing that last test fail, returning a BC entity from getOwner() to be consistent with other places.
Comment #53
moshe weitzman CreditAttribution: moshe weitzman commentedDo we usually add an @file and/or Doxygen for interfaces like this?
This patch is huge, but it is quite a straightforward conversion, by now. This holds up REST support for imagefield, so lets get it in. Proceeding with RTBC since my point above could not be more trivial.
Comment #54
BerdirRe-roll, didn't apply anymore, conflict in ImageAdminStylesTest.php due to the removed manifest code.
Removing the avoid commit conflicts tag, I have other rtbc issues that are just as important and I know that certain relevant people don't like it ;)
Comment #55
fagoAs always, great work!
This line shows very well what's weird with this path: We use methods for read access, but not for write access? That seems inconsistent to me. I think we should either use the methods always - or never.
Consequently, I think we should have setters for everything that has a getter and is writeable.
Those methods look great, but I think we should clarify the relationship to the respective entity fields. Is the method supposed to be an abstraction from the field used? I don't think so, imho it should always map to the field - else it becomes confusing when the field and methods differ.
Thus, imo we should document that clearly - not sure we do that best though? Maybe just note it for the interface in general that it adds accessor methods to file entity base fields?
Comment #56
BerdirI wasn't sure about write methods, there are very few cases where we actually change these and almost all of those are in functions that should be methods on FileInterface I think (like file_copy/file_move), so I wasn't sure about adding code for something will almost never be used outside of the file entity. Crell linked to an interesting article that said you should have methods for specific uses cases instead of general-purpose setters. But unlike application-code, we can't foresee all use cases, so it probably doesn't hurt to have those methods ;)
I see your point, but I have no idea how to document it better :)
Comment #57
BerdirAdded a few setters.
Comment #59
BerdirIt would help a lot if I'd use the correct method name. This should be green. If testbot behaves.
@moshe. FileInterface.php has a correct @file docblock, it's just not visible in the patch as I'm not adding that interface, I'm just adding methods to the currently empty interface.
Comment #60
fagoSo how about adding this to the interface doc-block?
doubled @
getMime()? That sounds weird to me. I'd call that getMimeType() - might be a personal preference though.
This should mention the constants for those values I guess.
This duplicates the isPermanent() and isTemporary() methods a bit - but only on read, not on write. Do we need getStatus() in addition to the oterhs? If so, I think it should have setStatus() also.
Documentation and method name do not match.
Misses setter.
Two many empty lines.
Comment #61
BerdirWell, the interface might be used for more than just getters and setters, what about the file_copy and file_move() functions for example? I'd love to see stuff like that as a method on that interface...
The thing with getStatus() is that per the existing documentation, core defines 0 and 1 but leaves it open for contrib but that's weird anyway. So I killed that for now, @timplunkett has a EntityStatusInterface that would add it for all entities but as long as the meaning is different that's tricky too.. but I'll leave it for that issue to re-add it if we want to.
Renamed to get/setMimeType(), we also have a getMimeType() on the stream wrapper interface, so this makes sense.
Comment #63
BerdirMissed a getStatus().
Comment #65
BerdirDon't we love it if a 180kb patch conflicts because of a simple use change ;)
Comment #66
fagotrue, but then - couldn't we just rephrase it to something like:
?
Howsoever, improvements look good and this is ready to fly. Great work! (once again)
Comment #67
BerdirRe-roll. Changed the interface name without the suffix as that we would be too long and we don't have those additional methods.. yet.
conflicted in file_test because that was converted to a controller.
Comment #68
BerdirWeird double post.
Comment #69
BerdirDidn't mean to set it back to RTBC myself :)
Not sure what's up with the testbot...
Comment #71
Berdir#67: file-entity-ng-1818568-67.patch queued for re-testing.
Comment #72
das-peter CreditAttribution: das-peter commentedCompared the two patches manually and checked the interdiff.
Changes look good / consistent and since this was RTBC (#66) already I'd say RTBC again :)
Comment #73
alexpottCommitted 6d54ed7 and pushed to 8.x. Thanks!
Comment #74
BerdirUpdated the existing change notice, linked to the interfaces and said a bit about the interfaces/methods: https://drupal.org/node/1806650/revisions/view/2719291/2729057
So I think we're good here.
Comment #76
xjmUntagging. Please remove the tag when the change notification task is completed.
Comment #77
xjmd.o--
Comment #78
amateescu CreditAttribution: amateescu commentedI wonder why this patch made 'filesize' a boolean field. #2149877: The 'filesize' base field of field entities is a boolean?