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.

CommentFileSizeAuthor
#68 file-entity-ng-1818568-67-interdiff.txt1.8 KBBerdir
#67 file-entity-ng-1818568-67.patch184.65 KBBerdir
#67 file-entity-ng-1818568-67-interdiff.txt1.8 KBBerdir
#65 file-entity-ng-1818568-65.patch184.2 KBBerdir
#63 file-entity-ng-1818568-63.patch184.12 KBBerdir
#63 file-entity-ng-1818568-63-interdiff.txt1.45 KBBerdir
#61 file-entity-ng-1818568-61.patch184.11 KBBerdir
#61 file-entity-ng-1818568-61-interdiff.txt22.03 KBBerdir
#59 file-entity-ng-1818568-59.patch183.84 KBBerdir
#59 file-entity-ng-1818568-59-interdiff.txt939 bytesBerdir
#57 file-entity-ng-1818568-57.patch183.85 KBBerdir
#57 file-entity-ng-1818568-57-interdiff.txt13.82 KBBerdir
#54 file-entity-ng-1818568-54.patch181.14 KBBerdir
#52 file-entity-ng-1818568-52.patch181.11 KBBerdir
#52 file-entity-ng-1818568-52-interdiff.txt524 bytesBerdir
#50 file-entity-ng-1818568-50.patch181.1 KBBerdir
#50 file-entity-ng-1818568-50-interdiff.txt5.07 KBBerdir
#48 file-entity-ng-1818568-48.patch181.1 KBBerdir
#48 file-entity-ng-1818568-48-interdiff.txt936 bytesBerdir
#46 file-entity-ng-1818568-46.patch181.1 KBBerdir
#46 file-entity-ng-1818568-46-interdiff.txt716 bytesBerdir
#44 file-entity-ng-1818568-44.patch181.11 KBBerdir
#44 file-entity-ng-1818568-44-interdiff.txt16.18 KBBerdir
#41 file-entity-ng-1818568-41.patch179.38 KBBerdir
#41 file-entity-ng-1818568-41-interdiff.txt1.69 KBBerdir
#39 file-entity-ng-1818568-39.patch178.3 KBBerdir
#39 file-entity-ng-1818568-39-interdiff.txt38.09 KBBerdir
#34 file-entity-ng-1818568-34.patch177.31 KBBerdir
#30 file-entity-ng-1818568-30.patch177.29 KBBerdir
#30 file-entity-ng-1818568-30-interdiff.txt3.71 KBBerdir
#29 file-entity-ng-1818568-29.patch178.42 KBBerdir
#28 file-entity-ng-1818568-28.patch173.07 KBdas-peter
#28 interdiff-1818568-26-28-do-not-test.diff3.4 KBdas-peter
#26 file-entity-ng-1818568-26.patch170.67 KBdas-peter
#26 interdiff-1818568-24-26-do-not-test.diff669 bytesdas-peter
#24 file-entity-ng-1818568-24.patch168.64 KBBerdir
#23 file-entity-ng-1818568-23.patch170.42 KBdas-peter
#23 interdiff--1818568-21-23-do-not-test.diff2.54 KBdas-peter
#21 file-entity-ng-1818568-21.patch168.21 KBBerdir
#21 file-entity-ng-1818568-21-interdiff.txt7.41 KBBerdir
#17 file-entity-ng-1818568-17.patch163.14 KBBerdir
#17 file-entity-ng-1818568-16.patch11.37 KBBerdir
#15 file-entity-ng-1818568-15.patch171.38 KBBerdir
#15 file-entity-ng-1818568-15-interdiff.txt466 bytesBerdir
#11 file-entity-ng-1818568-11.patch171.33 KBBerdir
#11 file-entity-ng-1818568-11-interdiff.txt10.45 KBBerdir
#9 file-entity-ng-1818568-9.patch163.1 KBBerdir
#9 file-entity-ng-1818568-9-interdiff.txt15.61 KBBerdir
#7 file-entity-ng-1818568-6.patch153.57 KBBerdir
#5 file-entity-ng-1818568-5.patch153.57 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Issue tags: +Entity Field API

adding tag

Gábor Hojtsy’s picture

Just 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.

Anonymous’s picture

I should note that this is a priority for REST, though. We want to support RESTfully creating file entities (like images for image fields).

Crell’s picture

Priority: Normal » Major
Issue tags: +WSCCI
Berdir’s picture

Status: Active » Needs review
FileSize
153.57 KB

Here'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.

Status: Needs review » Needs work

The last submitted patch, file-entity-ng-1818568-5.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
153.57 KB

Patch didn't apply anymore.

Status: Needs review » Needs work

The last submitted patch, file-entity-ng-1818568-6.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
15.61 KB
163.1 KB

Fixing some tests. Ugly stuff in locale.module :(

Status: Needs review » Needs work

The last submitted patch, file-entity-ng-1818568-9.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
10.45 KB
171.33 KB

More test fixes. Most except the locale stuff should be fixed.

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

The last submitted patch, file-entity-ng-1818568-11.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#11: file-entity-ng-1818568-11.patch queued for re-testing.

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

The last submitted patch, file-entity-ng-1818568-11.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
466 bytes
171.38 KB

Forgot the use there.

Status: Needs review » Needs work

The last submitted patch, file-entity-ng-1818568-15.patch, failed testing.

Berdir’s picture

Ok, 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.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, file-entity-ng-1818568-16.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

-16 was the interdiff. Not sure what I was thinking there..

Berdir’s picture

This might be green.

Status: Needs review » Needs work

The last submitted patch, file-entity-ng-1818568-21.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
2.54 KB
170.42 KB

I 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.

Berdir’s picture

Re-roll, no other changes yet.

Status: Needs review » Needs work

The last submitted patch, file-entity-ng-1818568-24.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
669 bytes
170.67 KB

Fixed the re-roll, there were some deprecated constants leftover.

Status: Needs review » Needs work

The last submitted patch, file-entity-ng-1818568-26.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
3.4 KB
173.07 KB
+++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldPathTest.php
@@ -46,8 +46,7 @@ function testUploadPath() {
-    $node_file = file_load($node->{$field_name}[Language::LANGCODE_NOT_SPECIFIED][0]['fid']);

We'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.

Berdir’s picture

As 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.

Berdir’s picture

Yeah, 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...

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

The last submitted patch, file-entity-ng-1818568-30.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#30: file-entity-ng-1818568-30.patch queued for re-testing.

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

The last submitted patch, file-entity-ng-1818568-30.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
177.31 KB

Re-roll.

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

The last submitted patch, file-entity-ng-1818568-34.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#34: file-entity-ng-1818568-34.patch queued for re-testing.

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

The last submitted patch, file-entity-ng-1818568-34.patch, failed testing.

msonnabaum’s picture

Great work. It's nice to see FilesInterface get some methods :)

public function getFileUri() {
  return $this->get('uri')->value;
}

public function getFileMime() {
  return $this->get('filemime')->value;
}

public function getFileSize() {
  return $this->get('filesize')->value;
}

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.

 /**
  * Returns the timestamp when the file was created.
  *
  * @return int
  *   Creation timestamp of the file.
  */
  public function getTimestamp();

I'd probably name that something like getCreatedTime(), getCreatedTimestamp(), or maybe even createdAt(), to better reflect what that property returns.

- Have not yet added one for the uid, as there's targetId and entity, not sure what to do. GetUid()/getUser(), getUser/getUserId()?

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().

Berdir’s picture

Status: Needs work » Needs review
FileSize
38.09 KB
178.3 KB

Thanks.

- 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.

Status: Needs review » Needs work

The last submitted patch, file-entity-ng-1818568-39.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
179.38 KB

And this should fix the last test failure there.

moshe weitzman’s picture

setPermanent() sounds like a good method name to me.

msonnabaum’s picture

Temp almost feels like a different object, but I think isPermanent/isTemporary and setPermanent/setTemporary are maybe the best we can do with methods.

Berdir’s picture

Sure 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.

Status: Needs review » Needs work

The last submitted patch, file-entity-ng-1818568-44.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
716 bytes
181.1 KB

Stupid mistakes are stupid.

Status: Needs review » Needs work

The last submitted patch, file-entity-ng-1818568-46.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
936 bytes
181.1 KB

This should be green again.

Berdir’s picture

Just noticed that timestamp is actually the changed timestamp, so the method name isn't correct.

Berdir’s picture

Changed that to getChangedTime().

Status: Needs review » Needs work

The last submitted patch, file-entity-ng-1818568-50.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
524 bytes
181.11 KB

Fixing that last test fail, returning a BC entity from getOwner() to be consistent with other places.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Avoid commit conflicts
+++ b/core/modules/file/lib/Drupal/file/FileInterface.phpundefined
+++ b/core/modules/file/lib/Drupal/file/FileInterface.phpundefined
@@ -14,4 +14,95 @@

@@ -14,4 +14,95 @@
  */
 interface FileInterface extends ContentEntityInterface {
 

Do 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.

Berdir’s picture

Re-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 ;)

fago’s picture

Status: Reviewed & tested by the community » Needs work

As always, great work!

+++ b/core/modules/file/file.module
@@ -175,34 +175,33 @@ function file_usage() {
+        $file->filename->value = $existing->getFilename();

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.

+++ b/core/modules/file/lib/Drupal/file/FileInterface.php
@@ -14,4 +14,95 @@
+   * Returns the size of the file.
+   *
+   * @return string
+   *   The size of the file in bytes.
+   */
+  public function getSize();

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?

Berdir’s picture

I 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 :)

Berdir’s picture

Status: Needs work » Needs review
FileSize
13.82 KB
183.85 KB

Added a few setters.

Status: Needs review » Needs work

The last submitted patch, file-entity-ng-1818568-57.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
939 bytes
183.84 KB

It 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.

fago’s picture

Status: Needs review » Needs work

I see your point, but I have no idea how to document it better :)

So how about adding this to the interface doc-block?

Defines getter and setter methods for file entity base fields.

+++ b/core/modules/file/lib/Drupal/file/FileInterface.php
@@ -14,4 +14,129 @@
+   * @@param string $uri

doubled @

+++ b/core/modules/file/lib/Drupal/file/FileInterface.php
@@ -14,4 +14,129 @@
+  public function getMime();

getMime()? That sounds weird to me. I'd call that getMimeType() - might be a personal preference though.

+++ b/core/modules/file/lib/Drupal/file/FileInterface.php
@@ -14,4 +14,129 @@
+   *  1 for permanent files, 0 for temporary.

This should mention the constants for those values I guess.

+++ b/core/modules/file/lib/Drupal/file/FileInterface.php
@@ -14,4 +14,129 @@
+  public function getStatus();

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.

+++ b/core/modules/file/lib/Drupal/file/FileInterface.php
@@ -14,4 +14,129 @@
+   * Returns the timestamp when the file was created.
+   *
+   * @return int
+   *   Creation timestamp of the file.
+   */
+  public function getChangedTime();

Documentation and method name do not match.

+++ b/core/modules/file/lib/Drupal/file/FileInterface.php
@@ -14,4 +14,129 @@
+  /**
+   * Returns the user that owns this file.
+   *
+   * @return \Drupal\user\UserInterface
+   *   The user that owns the file.
+   */

Misses setter.

+++ b/core/modules/file/lib/Drupal/file/FileInterface.php
@@ -14,4 +14,129 @@
+   *   The user that owns the file.
...
+
+

Two many empty lines.

Berdir’s picture

Status: Needs work » Needs review
FileSize
22.03 KB
184.11 KB

Well, 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.

Status: Needs review » Needs work

The last submitted patch, file-entity-ng-1818568-61.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.45 KB
184.12 KB

Missed a getStatus().

Status: Needs review » Needs work

The last submitted patch, file-entity-ng-1818568-63.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
184.2 KB

Don't we love it if a 180kb patch conflicts because of a simple use change ;)

fago’s picture

Status: Needs review » Reviewed & tested by the community

Well, 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...

true, but then - couldn't we just rephrase it to something like:

Defines getter and setter methods for file entity base fields, among other file related methods.

?

Howsoever, improvements look good and this is ready to fly. Great work! (once again)

Berdir’s picture

Re-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.

Berdir’s picture

Weird double post.

Berdir’s picture

Status: Reviewed & tested by the community » Needs review

Didn't mean to set it back to RTBC myself :)

Not sure what's up with the testbot...

Status: Needs review » Needs work
Issue tags: -WSCCI, -Entity Field API, -Field API NG blocker

The last submitted patch, file-entity-ng-1818568-67.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI, +Entity Field API, +Field API NG blocker

#67: file-entity-ng-1818568-67.patch queued for re-testing.

das-peter’s picture

Status: Needs review » Reviewed & tested by the community

Compared the two patches manually and checked the interdiff.
Changes look good / consistent and since this was RTBC (#66) already I'd say RTBC again :)

alexpott’s picture

Title: Convert files to the new Entity Field API » Change notice: Convert files to the new Entity Field API
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 6d54ed7 and pushed to 8.x. Thanks!

Berdir’s picture

Title: Change notice: Convert files to the new Entity Field API » Convert files to the new Entity Field API
Priority: Critical » Major
Status: Active » Fixed

Updated 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.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the tag when the change notification task is completed.

xjm’s picture

d.o--

amateescu’s picture

I wonder why this patch made 'filesize' a boolean field. #2149877: The 'filesize' base field of field entities is a boolean?