Problem/Motivation

API page: https://api.drupal.org/api/drupal/core%21includes%21file.inc/group/file/...

The whole section 'Fields on the file entity:' is obsolete, as files are now entities, and as such you need to use methods to access their properties.

Might this whole topic be obsolete?

Proposed resolution

  • Remove the 'Fields on the file entity' documentation from file.inc, leaving the @defgroup tags in place.
  • Add @ingroup file to FileInterface and File so they appear in the group page on api.drupal.org.
  • Ensure that coding standards tag order is proper: https://www.drupal.org/node/1354#order

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#17 interdiff-2721887-13-17.txt522 bytesashishdalvi
#17 2721887-17.patch1.93 KBashishdalvi
#13 interdiff-2721887-9-13.txt1.09 KBSonal.Sangale
#13 2721887-13.patch1.92 KBSonal.Sangale
#9 interdiff.txt1.01 KBsnehi
#9 2721887-9.patch1.88 KBsnehi
#6 interdiff.txt274 bytesgaurav.pahuja
#6 2721887-6.patch1.17 KBgaurav.pahuja
#3 2721887-3.patch1.14 KBsnehi

Comments

joachim created an issue. See original summary.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue tags: +Novice

Agreed that the "Fields on the file entity" section should be removed.

However, I think we still need the @defgroup, because it is a place to collect various file-handling functions. So let's just take out that part of the doc block.

As a replacement, the FileInterface and File class (from the File module), should have @ingroup file added to their doc block, so that they appear on this page.

snehi’s picture

snehi commented
Status: Active » Needs review
StatusFileSize
new 2721887-3.patch1.14 KB

Created the first patch for this issue.
As said by Jhodgdon @defgroup is still there.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

See the end of comment #2 please -- we need also to add @ingroup to a few classes/interfaces.

gaurav.pahuja’s picture

gaurav.pahuja commented
Assigned: Unassigned » gaurav.pahuja
gaurav.pahuja’s picture

gaurav.pahuja commented
Status: Needs work » Needs review
StatusFileSize
new 2721887-6.patch1.17 KB
new interdiff.txt274 bytes

Giving it a try.

gaurav.pahuja’s picture

gaurav.pahuja commented
Assigned: gaurav.pahuja » Unassigned
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work 
+++ b/core/includes/file.inc
@@ -31,25 +31,9 @@
+ * @ingroup file

No, not here -- the @ingroup file needs to be added to the File entity class and its FileInterface interface. These are in the file module.

snehi’s picture

snehi commented
Status: Needs work » Needs review
StatusFileSize
new 2721887-9.patch1.88 KB
new interdiff.txt1.01 KB

Uploading new patch.

Status: Needs review » Needs work

The last submitted patch, 9: 2721887-9.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

The test failure is unrelated... but the @ingroup lines are in the wrong places:

a) See https://www.drupal.org/node/1354#order -- they must be at the end of the doc block.

b) The one in FileInterface seems to be in a method's doc block instead of in the interface's doc block.

Sonal.Sangale’s picture

Sonal.Sangale commented
Assigned: Unassigned » Sonal.Sangale
Sonal.Sangale’s picture

Sonal.Sangale commented
Status: Needs work » Needs review
StatusFileSize
new 2721887-13.patch1.92 KB
new interdiff-2721887-9-13.txt1.09 KB

Given it try!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Almost!

+++ b/core/modules/file/src/Entity/File.php
@@ -30,6 +30,8 @@
+ *
+ * @ingroup file

Please see
https://www.drupal.org/node/1354#order again. The entity annotation has to come last in the doc block. @see goes before that. Sorry for saying "comes last", it's not quite right.

Sonal.Sangale’s picture

Sonal.Sangale commented
Assigned: Sonal.Sangale » Unassigned
ashishdalvi’s picture

ashishdalvi
Primary language English
Location Mumbai
commented
Assigned: Unassigned » ashishdalvi

I am taking this issue.

ashishdalvi’s picture

ashishdalvi
Primary language English
Location Mumbai
commented
Assigned: ashishdalvi » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2721887-17.patch1.93 KB
new interdiff-2721887-13-17.txt522 bytes

Thank you jhodgdon for reference links. It helps lot to improve drupal skills.

Adding patch and interdiff based on the suggestion in comment #14

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Version: 8.2.x-dev » 8.1.x-dev
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Updated issue summary with the instructions from the comment thread.

The patch applies, removes the wrong stuff, adds the right stuff, and passes tests.

So yay, RTBC. :-)

The patch also applies cleanly to 8.1.x, and since it's a documentation-only fix, I'm moving it to 8.1.x and re-running tests. But obviously maintainers can apply it wherever is most appropriate.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

+1 for RTBC - thanks!

  • xjm committed 8692778 on 8.2.x 
    Issue #2721887 by snehi, Sonal.Sangale, gaurav.pahuja, Ashish.Dalvi,...

  • xjm committed c850275 on 8.1.x 
    Issue #2721887 by snehi, Sonal.Sangale, gaurav.pahuja, Ashish.Dalvi,...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Cleaning it up this way makes sense to me too. Thanks everyone! Committed to 8.2.x and cherry-picked to 8.1.x.

Status: Fixed » Closed (fixed)

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