Follow up to #2244513: Move the unmanaged file APIs to the file_system service (file.inc), we need to replace usages of deprecated constants in file.inc with those on FileSystemInterface.

Issue fork drupal-3023015

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

kim.pepper created an issue. See original summary.

berdir’s picture

Status: Postponed » Active

This is unblocked now.

berdir’s picture

Status: Active » Needs review
StatusFileSize
new41.92 KB

Replaced all usages that I found of "FILE_EXISTS" with "FileSystemInterface::EXISTS", except those in file.inc on the deprecated functions.

jibran’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record updates

Applied the patch locally and had a look and only found contrib usages. We should update the original change record https://www.drupal.org/node/3006851.

kim.pepper’s picture

I updated the change record to list the constant changes.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: file-deprecated-contents-3023015-3.patch, failed testing. View results

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record updates
StatusFileSize
new42.02 KB

Missed when this conflicted, rerolled.

Change record was already updated, so removing that tag. This just does the rename, it doesn't introduce anything new.

berdir’s picture

StatusFileSize
new42.2 KB

Lots of conflicts but all pretty simple to resolve.

berdir’s picture

mikelutz’s picture

Status: Needs review » Needs work

All looks good, only a few documentation nits.

  1. +++ b/core/lib/Drupal/Core/File/FileSystemInterface.php
    @@ -281,8 +281,9 @@ public function validScheme($scheme);
    -   *   $replace parameter. FILE_EXISTS_REPLACE will error out.
    -   *   FILE_EXISTS_RENAME will rename the file until the $destination is unique.
    +   *   $replace parameter. FileSystemInterface::EXISTS_REPLACE will error out.
    +   *   FileSystemInterface::EXISTS_RENAME will rename the file until the
    +   *   $destination is unique.
    
    +++ b/core/modules/file/file.module
    @@ -129,8 +129,9 @@ function file_load($fid, $reset = FALSE) {
      * - If the $source and $destination are equal, the behavior depends on the
    - *   $replace parameter. FILE_EXISTS_REPLACE will error out. FILE_EXISTS_RENAME
    - *   will rename the file until the $destination is unique.
    + *   $replace parameter. FileSystemInterface::EXISTS_REPLACE will error out.
    + *   FileSystemInterface::EXISTS_RENAME will rename the file until the
    + *   $destination is unique.
    

    I realize you didn't change this, but this doesn't look right.. EXISTS_REPLACE should replace, EXISTS_ERROR should throw an error, this line conflicts with the @param int $replace documentation below..

  2. +++ b/core/lib/Drupal/Core/File/FileSystemInterface.php
    @@ -367,10 +368,10 @@ public function deleteRecursive($path, callable $callback = NULL);
    +   *   - FileSystemInterface::EXISTS_RENAME - Append _{incrementing number} until the filename
    

    nit: over 80 characters

  3. +++ b/core/modules/file/file.module
    @@ -143,12 +144,12 @@ function file_load($fid, $reset = FALSE) {
    + *   - FileSystemInterface::EXISTS_REPLACE: Replace the existing file. If a managed file with
    ...
    + *   - FileSystemInterface::EXISTS_RENAME: (default) Append _{incrementing number} until the
    

    nit: over 80 characters.

  4. +++ b/core/modules/file/file.module
    @@ -225,13 +226,13 @@ function file_copy(FileInterface $source, $destination = NULL, $replace = FILE_E
    + *   - FileSystemInterface::EXISTS_REPLACE: Replace the existing file. If a managed file with
    ...
    + *   - FileSystemInterface::EXISTS_RENAME: (default) Append _{incrementing number} until the
    

    nit: 80 characters

  5. +++ b/core/modules/file/file.module
    @@ -551,19 +552,19 @@ function file_validate_image_resolution(FileInterface $file, $maximum_dimensions
    + *   - FileSystemInterface::EXISTS_REPLACE: Replace the existing file. If a managed file with
    ...
    + *   - FileSystemInterface::EXISTS_RENAME: (default) Append _{incrementing number} until the
    

    nit: 80 characters

  6. +++ b/core/modules/file/file.module
    @@ -778,10 +779,10 @@ function file_cron() {
    + *   - FileSystemInterface::EXISTS_RENAME: (default) Append _{incrementing number} until the
    
    @@ -877,10 +878,10 @@ function _file_save_upload_from_form(array $element, FormStateInterface $form_st
    + *   - FileSystemInterface::EXISTS_RENAME: (default) Append _{incrementing number} until the
    

    nit: 80 characters

  7. +++ b/core/modules/system/system.module
    @@ -1341,10 +1341,10 @@ function system_time_zones($blank = NULL, $grouped = FALSE) {
    + *   - FileSystemInterface::EXISTS_RENAME: Append _{incrementing number} until the filename is
    

    nit: 80 characters

  8. One more place in documentation that needs attention (I'm guessing it's new):

    Drupal\jsonapi\Controller\TemporaryJsonapiFileFieldUploader:190

        // Move the file to the correct location after validation. Use
        // FILE_EXISTS_ERROR as the file location has already been determined above
        // in FileSystem::getDestinationFilename().
        try {
          $this->fileSystem->move($temp_file_path, $file_uri, FileSystemInterface::EXISTS_ERROR);
        }
        catch (FileException $e) {
          throw new HttpException(500, 'Temporary file could not be moved to file location');
        }
    

Also, what are we doing with FILE_STATUS_PERMANENT? It's not deprecated or on the interface yet. Is there another issue for that one?

kim.pepper’s picture

Status: Needs work » Needs review
StatusFileSize
new43.92 KB
new7.28 KB

Fixes for #11

  1. Fixed
  2. Fixed
  3. Fixed
  4. Fixed
  5. Fixed
  6. Fixed
  7. Fixed
  8. Fixed

See #3021833: Move FILE_STATUS_PERMANENT to \Drupal\file\FileInterface

mikelutz’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/File/FileSystem.php
@@ -446,10 +446,10 @@ public function move($source, $destination, $replace = self::EXISTS_RENAME) {
+   *   - FileSystemInterface::EXISTS_RENAME - Append _{incrementing number} until the filename

+++ b/core/modules/file/file.module
@@ -877,10 +881,10 @@ function _file_save_upload_from_form(array $element, FormStateInterface $form_st
+ *   - FileSystemInterface::EXISTS_RENAME: (default) Append _{incrementing number} until the

nit: looks like two more 80+ char comments, I must have missed in my initial review. :-/

tinko’s picture

Status: Needs work » Needs review
StatusFileSize
new44.01 KB

Hello,

I've added the changes for comments to the previous patch.

:)

kim.pepper’s picture

StatusFileSize
new2.46 KB

Missing interdiff for #14

tinko’s picture

Thank you, kim.pepper. I'll try to not forget it next time. :)

kim.pepper’s picture

No worries! Thanks for your patch!

mikelutz’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. You even found one other comment to fix which I had missed!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 9aa7d13c9f and pushed to 8.8.x. Thanks!

  • alexpott committed 9aa7d13 on 8.8.x
    Issue #3023015 by Berdir, kim.pepper, tinko, mikelutz: Replace usages of...

Status: Fixed » Closed (fixed)

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

adsyy made their first commit to this issue’s fork.