Move FILE_STATUS_PERMANENT to \Drupal\file\FileInterface

I like STATUS_PERMANENT. :-)

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -567,7 +567,7 @@
+ *   ->condition('status', \Drupal\file\FileInterface::STATUS_PERMANENT, '<>')

Can import the class.

I don't think the constant is really "used", but it *is* in file.inc, which is always loaded.

Just don't use the constant there and keep the old hardcoded value. Loading the constants there defeats the point of having them lazy-loaded on the interface anyway (just for BC, but still).

It's a constant, there is no reason to have it dynamic somehow.

Did as suggested in #8 and converted FILE_STATUS_PERMANENT back to static variable.

> not clear how that change can affect tests while this constant is not used

But it *is* used right there, simply by loading that file, and that file is always loaded. The constant is defined there, based on the other constant, so it has to load that.

1. +++ b/core/includes/file.inc
   @@ -72,6 +73,11 @@
   + *
   + * @deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0.
   + *   Use \Drupal\file\FileInterface::STATUS_PERMANENT instead.
   + *
   + * @see https://www.drupal.org/project/drupal/issues/3021833
   

   Instead of a @see, we need an change record for this and then the @deprecated line needs to point to that.

2. +++ b/core/modules/editor/editor.module
   @@ -440,8 +441,8 @@ function editor_entity_revision_delete(EntityInterface $entity) {
    function _editor_record_file_usage(array $uuids, EntityInterface $entity) {
      foreach ($uuids as $uuid) {
        if ($file = \Drupal::entityManager()->loadEntityByUuid('file', $uuid)) {
   -      if ($file->status !== FILE_STATUS_PERMANENT) {
   -        $file->status = FILE_STATUS_PERMANENT;
   +      if ($file->status !== FileInterface::STATUS_PERMANENT) {
   +        $file->status = FileInterface::STATUS_PERMANENT;
            $file->save();
          }
   

   I remember this code from another issue, this is broken because $file->status is an object.

   And it's extra broken because it is a type safe against an entity value, which aren't type safe.

   Instead of just doing a 1:1 replacement of the constant, lets use isPermanent()/isTemporary() here, this will be much easier to read. Also the comment above doesn't really need to refer the constant, it's then also clear enough of we just write "is not yet permanent".

1. +++ b/core/includes/file.inc
   @@ -77,7 +76,7 @@
     * @deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0.
     *   Use \Drupal\file\FileInterface::STATUS_PERMANENT instead.
     *
   - * @see https://www.drupal.org/project/drupal/issues/3021833
   + * @see https://www.drupal.org/node/3022147
   

   I thought it needs to be part of the @deprecated text, not @see. but that could be fixed on commit if necessary.

2. +++ b/core/modules/editor/editor.module
   @@ -430,8 +430,7 @@ function editor_entity_revision_delete(EntityInterface $entity) {
     *
   - * Every referenced file that does not yet have the
   - * \Drupal\file\FileInterface::STATUS_PERMANENT state, will be given that state.
   + * Every referenced file that is temporally saved will be resaved as permanent.
   

   "temporary", also "saved" is not needed. Maybe also something to be fixed on commit, but someone could also upload an updated patch while it's RTBC.

So for me we should consider completely removing this constant. I wonder what @Berdir thinks.

1. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationSyncImageTest.php
   @@ -138,7 +139,7 @@ public function testImageFieldSync() {
   -        'status' => FILE_STATUS_PERMANENT,
   +        'status' => FileInterface::STATUS_PERMANENT,
   
   +++ b/core/modules/file/tests/src/Functional/FileListingTest.php
   @@ -214,7 +215,7 @@ protected function createFile() {
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   
   +++ b/core/modules/file/tests/src/Functional/FileManagedAccessTest.php
   @@ -35,7 +36,7 @@ public function testFileAccess() {
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   
   @@ -60,7 +61,7 @@ public function testFileAccess() {
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   
   +++ b/core/modules/file/tests/src/Kernel/AccessTest.php
   @@ -131,7 +132,7 @@ public function testFileCacheability() {
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   
   +++ b/core/modules/file/tests/src/Kernel/FileUriItemTest.php
   @@ -26,7 +27,7 @@ public function testCustomFileUriField() {
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   
   +++ b/core/modules/file/tests/src/Kernel/Migrate/d6/MigrateUploadTest.php
   @@ -40,7 +41,7 @@ protected function setUp() {
   -        'status' => FILE_STATUS_PERMANENT,
   +        'status' => FileInterface::STATUS_PERMANENT,
   
   +++ b/core/modules/file/tests/src/Kernel/SaveTest.php
   @@ -18,7 +19,7 @@ public function testFileSave() {
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   
   @@ -58,7 +59,7 @@ public function testFileSave() {
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   
   @@ -88,7 +89,7 @@ public function testFileSave() {
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   
   +++ b/core/modules/hal/tests/src/Kernel/FileNormalizeTest.php
   @@ -34,7 +35,7 @@ public function testNormalize() {
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   
   +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
   @@ -86,7 +87,7 @@ public function testImageInPlaceEditor() {
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   
   +++ b/core/modules/image/tests/src/Kernel/Views/RelationshipUserImageDataTest.php
   @@ -69,7 +70,7 @@ public function testViewsHandlerRelationshipUserImageData() {
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   
   +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockPrivateFilesTest.php
   @@ -218,7 +218,7 @@ protected function createPrivateFile($file_name) {
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   
   +++ b/core/modules/locale/tests/src/Functional/LocaleUpdateBase.php
   @@ -137,7 +138,7 @@ protected function makePoFile($path, $filename, $timestamp = NULL, array $transl
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   
   +++ b/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserTest.php
   @@ -45,7 +46,7 @@ protected function setUp() {
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   
   @@ -59,7 +60,7 @@ protected function setUp() {
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   
   +++ b/core/modules/views/tests/src/Functional/Plugin/NumericFormatPluralTest.php
   @@ -148,7 +149,7 @@ protected function createFile() {
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   

   Let's change all of these to $file->setPermanent()

2. +++ b/core/modules/file/file.module
   @@ -562,7 +562,7 @@ function file_save_data($data, $destination = NULL, $replace = FILE_EXISTS_RENAM
   -      'status' => FILE_STATUS_PERMANENT,
   +      'status' => FileInterface::STATUS_PERMANENT,
   

   Consider replacing with ->setPermanent() on the entity once created.

3. +++ b/core/modules/file/file.module
   @@ -1730,7 +1730,7 @@ function file_get_file_references(FileInterface $file, FieldDefinitionInterface
        0 => t('Temporary'),
   -    FILE_STATUS_PERMANENT => t('Permanent'),
   +    FileInterface::STATUS_PERMANENT => t('Permanent'),
   

   For me hardcoding here would be fine - we've hardcoded the temporary after all.

4. +++ b/core/modules/file/src/Entity/File.php
   @@ -134,7 +134,7 @@ public function getCreatedTime() {
   -    return $this->get('status')->value == FILE_STATUS_PERMANENT;
   +    return $this->get('status')->value == FileInterface::STATUS_PERMANENT;
   
   @@ -148,7 +148,7 @@ public function isTemporary() {
   -    $this->get('status')->value = FILE_STATUS_PERMANENT;
   +    $this->get('status')->value = FileInterface::STATUS_PERMANENT;
   

   If we do choose to keep the constant then this could be static::STATUS_PERMANENT as this object implements the interface.

5. +++ b/core/modules/file/src/FileInterface.php
   @@ -13,6 +13,16 @@
   +   * Temporary files older than the system.file.temporary_maximum_age
   +   * configuration value will be, if clean-up not disabled, removed during cron
   +   * runs, but permanent files will not be removed during the file garbage
   +   * collection process.
   

   If we choose to remove the constant then we should move these docs to \Drupal\file\FileInterface::isPermanent(). And perhaps these docs belong there more anyway.

6. +++ b/core/modules/file/src/FileStorageInterface.php
   @@ -17,11 +17,11 @@
   -  public function spaceUsed($uid = NULL, $status = FILE_STATUS_PERMANENT);
   +  public function spaceUsed($uid = NULL, $status = FileInterface::STATUS_PERMANENT);
   

   This looks like one good reason to have the constant.

Hm, not sure, this constant isn't different to node status/sticky/promoted, user status, comment published, they all have such constants. We did convert them to constants on the interface, but they all date back a very long time.

Media on the other hand doesn't do that anymore and EntityPublishedInterface expects a boolean on setPublished (and the argument is deprecated anyway). And \Drupal\Core\Entity\EntityPublishedTrait::isPublished() just casts the value to a boolean. On the database level, it's 0/1, but the status/published fields are all of type boolean and then having value constants for that is indeed a bit strange.

permanent/temporary explains better than true/false or 0/1 what file status actually means, but most calls could indeed use the methods.

I don't like moving it to the storage, that's still different to all other entity types and I don't really want to promote custom storage implementations more than they already are, they just don't make for nice API's (hard to get correct autocomplete suggestions and things like that and every entity type with a custom storage class makes it harder for alternative storages to exist).

I agree with @Berdir that putting an entity value constant on the storage feels wrong because it is not storage related.

For the call is whether or not we should have the constant. If we should then let's put it on \Drupal\file\FileInterface.

I think this might be clearer once we convert everything to isPermanent()/isTemporary()/setPermanent()/setTemporary()

It looks much nicer with ->setPermanent(). Thanks.

+++ b/core/modules/file/tests/src/Kernel/SpaceUsedTest.php
@@ -63,15 +64,15 @@ public function testFileSpaceUsed() {
     $this->assertEqual($file->spaceUsed(1, 0), 0);
-    $this->assertEqual($file->spaceUsed(1, FILE_STATUS_PERMANENT), 0);
+    $this->assertEqual($file->spaceUsed(1, FileInterface::STATUS_PERMANENT), 0);

So for me stuff like this is really odd... ie. once with the (1, 0) and once with (1, FileInterface::STATUS_PERMANENT).

I also ponder whether we need the word STATUS<code>? i.e. <code>FileInterface::PERMANENT but then maybe this is odd with FileInterface::isPermanent() and FileInterface::setPermanent(). Not sure.

Re-roll of #22

Patch looks good.

1. +++ b/core/includes/file.inc
   @@ -92,6 +92,11 @@
    const FILE_STATUS_PERMANENT = 1;
   

   Can we change this line to: "const FILE_STATUS_PERMANENT = FileInterface::STATUS_PERMANENT".

2. +++ b/core/includes/file.inc
   @@ -92,6 +92,11 @@
   + * @deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0.
   

   Deprecation must now happen in 9.1.

@daffie #1 and #2 done on this patch

Patch fails to apply.

Error:

error: patch failed: core/includes/file.inc:90
error: core/includes/file.inc: patch does not apply

@pavnish: Did you do a "git pull"?

@daffie yes i am working with drupal 8.8 and simply apply the patch #26 and done the changes.
Please correct me if i am wrong.

@pavnish: The patch for 9.1 should go first and the testbot does not return green.

Edit: I did not want to unassign @pavnish from this issue.

diff --git a/core/includes/file.inc b/core/includes/file.inc
index 286c220..c27d6b1 100644
--- a/core/includes/file.inc
+++ b/core/includes/file.inc
@@ -7,6 +7,7 @@
 
 use Drupal\Component\Utility\UrlHelper;
 use Drupal\Core\StreamWrapper\StreamWrapperManager;
+use Drupal\file\FileInterface;
 
 /**
  * @defgroup file File interface
@@ -22,7 +23,7 @@
  * runs, but permanent files will not be removed during the file garbage
  * collection process.
  */
-const FILE_STATUS_PERMANENT = 1;
+const FILE_STATUS_PERMANENT = FileInterface::STATUS_PERMANENT;
 
 /**
  * Creates a web-accessible URL for a stream to an external or local file.

This change does not work. Please, change it back to:

diff --git a/core/includes/file.inc b/core/includes/file.inc
index edbd86c53f..c0c39b802e 100644
--- a/core/includes/file.inc
+++ b/core/includes/file.inc
@@ -92,6 +92,11 @@
  * configuration value will be, if clean-up not disabled, removed during cron
  * runs, but permanent files will not be removed during the file garbage
  * collection process.
...
+ * @deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0.
+ *   Use \Drupal\file\FileInterface::STATUS_PERMANENT instead.
+ *
+ * @see https://www.drupal.org/node/3022147
  */
 const FILE_STATUS_PERMANENT = 1;
 

I just did a re-roll of #26, updated the deprecation message version, and replaced a couple more usages of FILE_STATUS_PERMANENT in tests.

The interdiff is against #26.

Sorry @pavnish just saw you had assigned this to yourself.

@daffie same #42 has been changed

@ your patch looks good @kim.pepper

All occurrences of FILE_STATUS_PERMANENT have been replaced.
The constant has been properly deprecated.
The FileInterface has the replacement constant STATUS_PERMANENT.
The Is and the CR are in order.
All code changes look good to me.
For me it is RTBC.

Re-uploading the patch from comment #44 as that is the one that should be committed.

Adding reviewer credits.

I read the above discussion about the constant naming and whether we should have an interface constant, and I think the current patch is taking the right approach on both points.

1. +++ b/core/includes/file.inc
   @@ -21,6 +21,11 @@
   + * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use
   + *   \Drupal\file\FileInterface::STATUS_PERMANENT instead.
   

   Since setPermanent() is actually a better replacement whenever we have an instantiated file object, let's mention that in the deprecation message as well? (And in the CR.)

   We can probably just say:

   Use \Drupal\file\FileInterface::STATUS_PERMANENT or \Drupal\file\FileInterface::setPermanent() instead.

2. +++ b/core/modules/file/src/FileInterface.php
   @@ -13,6 +13,16 @@
   +   * Temporary files older than the system.file.temporary_maximum_age
   +   * configuration value will be, if clean-up not disabled, removed during cron
   +   * runs, but permanent files will not be removed during the file garbage
   +   * collection process.
   

   This is awkward wording. Since this is a deprecation (and therefore copied docs), but only a deprecation for this one exact thing, I could go either way on whether fixing the docs is in scope. Let's either fix it here or file a followup to fix it to:

   Temporary files older than the system.file.temporary_maximum_age will be removed during cron runs if cleanup is not disabled. (Permanent files will not be removed during the file garbage collection process.)

Both those points could be addressed by a novice contributor, so tagging accordingly. Thanks!

Hi,

Uploading the patch with the above changes has been addressed. Please verify the patch.

Fixed the spacing issue, Uploaded the interdiff from the previous patch. Please review

Updated the CR.

1 nitpick left:

+++ b/core/modules/file/src/FileInterface.php
@@ -13,6 +13,15 @@
+   * Temporary files older than the system.file.temporary_maximum_age
+   * will be removed during cron runs if cleanup is not disabled. (Permanent
+   * files will not be removed during the file garbage collection process.)

The comment is not fully using the maximum 80 character limit.

Thanks @Daffie, For quick review. Here is the updated patch.

@munish.kumar: I am missing the interdiff.txt. It makes reviewing your patch a lot easier.

Sorry @Daffie, I forgot to upload it. Here it is

@munish.kumar: Thank you for the interdiff.txt file.

All points of @xjm and myself are addressed.
Back to RTBC.

I changed the CR a bit to emphasize the public methods over the interface constant: https://www.drupal.org/node/3022147

I guess the deprecation message should also mention isPermanent().

Use
\Drupal\file\FileInterface::setPermanent(), \Drupal\file\FileInterface::isPermanent(), \Drupal\file\FileInterface::isTemporary(), or \Drupal\file\FileInterface::STATUS_PERMANENT instead.

Is that too long? We could shorten it to:

Use public FileInterface methods (preferred) or \Drupal\file\FileInterface::STATUS_PERMANENT instead.

...and then the linked CR details which methods.

That said. After doing that, I'm actually going back to earlier comments and questioning whether we actually want the new API to be a constant. Yes, as @Berdir points out this is the historical behavior, but isPermanent() and isTemporary() are a much clearer public API. Before, we didn't have an interface to make it explicit; now we do.

• Most calling code could be rewritten to check isPermanent() versus isTemporary().
• $file->status->value could become booleans instead of integers. I guess there's the contrib usecase of defining additional file status values, like FILE_STATUS_UNICORN = 2; But I'd argue File::isUnicorn() should go in a separate field.
• FileStorage::spaceUsed() is... weird, broken? It defaults to FILE_STATUS_PERMANENT for the second argument but doesn't say what you should pass instead if the file is not permanent. There's no FILE_STATUS_TEMPORARY so you'd have to pass a magic 0. Seems like that second arg could easily be converted to boolean as well, with @trigger_error()ed fallback code if an integer is passed.

Thoughts?

Patch looks good.
Just a small nitpick

+++ b/core/modules/file/tests/src/Kernel/SpaceUsedTest.php
@@ -63,15 +64,15 @@ public function testFileSpaceUsed() {
+    $this->assertEquals(370, $file->spaceUsed(NULL));

The parameter value NULL can be removed. As it is the default value.

Removed Default 'NULL' parameter from patch #67.

Patch is updated for 9.3.
The IS and the CR are in order.
All code changes look good to me.
There is no deprecation message test, because we are deprecating a constant.
For me it is RTBC.

Committed a227db1 and pushed to 9.3.x. Thanks!