Problem/Motivation

In #3123832: [META] Fix @todo items referencing closed issues we discovered that there's a @todo in \Drupal\file\Upload\FileUploadHandler::loadByUri which references a d.o. issue that is already closed; #3223209: deprecate file_save_data, file_copy and file_move and replace with a service

Here's the @todo:

  /**
   * Loads the first File entity found with the specified URI.
   *
   * @param string $uri
   *   The file URI.
   *
   * @return \Drupal\file\FileInterface|null
   *   The first file with the matched URI if found, NULL otherwise.
   *
   * @todo replace with https://www.drupal.org/project/drupal/issues/3223209
   */
  protected function loadByUri(string $uri): ?FileInterface {

Steps to reproduce

Proposed resolution

The current code in \Drupal\file\Upload\FileUploadHandler::loadByUri is almost similar to code that was changed here.

Let's apply the same changes to \Drupal\file\Upload\FileUploadHandler::loadByUri to keep consistent.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3337162

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:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Spokje created an issue. See original summary.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Issue summary: View changes

Spokje opened merge request !3316

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Status: Active » Needs review
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Change looks good and doesn't cause any issues so seems like a valid cleanup/update.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work

Added review to MR - we need to use dependency injection here.

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

_pratik_’s picture

_pratik_ CreditAttribution: _pratik_ as a volunteer and at Specbee for Drupal India Association commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work
Issue tags: +Needs change record

Now that the service has a new parameter it will need a change record.

Also new parameter should default to NULL and throw a trigger_error

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented

don't we need to deprecate it first as we are changing the service constructor params?

@deprecated in drupal:10.1.0 and is removed from drupal:11.0.0.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

So this is adding a parameter so would need something like

      @trigger_error('Calling ' . __CLASS__ . '::__construct() with the $migration argument is deprecated in drupal:10.1.0 and is removed in drupal:11.0.0. See https://www.drupal.org/node/3323212', E_USER_DEPRECATED);

With the URL being to the change record created here.

Then the test is maybe a Kernel test that shows you get that trigger_error when passing nothing in that last parameter spot.

paulocs opened merge request !3611

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Version: 10.0.x-dev » 10.1.x-dev
Status: Needs work » Needs review

Oh yes. I have read 'thrown error' instead of 'trigger_error' :)

I have created the deprecation notice. I have also created a new branch and MR as I can not change the target branch of MR !3316.

Spokje closed merge request !3316

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Looks good!

  • catch committed 283d9be7 on 10.1.x 
    Issue #3337162 by Spokje, paulocs, _pratik_, smustgrave, alexpott: Use...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Made this change on commit, otherwise looks great.

diff --git a/core/modules/file/src/Upload/FileUploadHandler.php b/core/modules/file/src/Upload/FileUploadHandler.php
index a79e757c67..ac997bcefa 100644
--- a/core/modules/file/src/Upload/FileUploadHandler.php
+++ b/core/modules/file/src/Upload/FileUploadHandler.php
@@ -120,7 +120,7 @@ public function __construct(FileSystemInterface $fileSystem, EntityTypeManagerIn
     $this->currentUser = $currentUser;
     $this->requestStack = $requestStack;
     if ($fileRepository === NULL) {
-      @trigger_error('Calling ' . __METHOD__ . ' without the $fileRepository argument is deprecated in drupal:10.1.5 and will be required in drupal:11.0.0. See https://www.drupal.org/node/3346839', E_USER_DEPRECATED);
+      @trigger_error('Calling ' . __METHOD__ . ' without the $fileRepository argument is deprecated in drupal:10.1.0 and will be required in drupal:11.0.0. See https://www.drupal.org/node/3346839', E_USER_DEPRECATED);
       $fileRepository = \Drupal::service('file.repository');
     }
     $this->fileRepository = $fileRepository;

Committed 283d9be and pushed to 10.1.x. Thanks!

  • catch committed 64d0d7e8 on 10.1.x 
    Issue #3337162 by Spokje, _pratik_, paulocs, smustgrave, alexpott: Use...

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

Published the CR