Problem/Motivation

Constructor has @deprecated tag and shouldn't.

it should trigger an error if the parameter is missing.
Additionally, DatabaseFileUsageBackend extends this class, and $config_factory is an optional third parameter in that class.

Proposed resolution

Remove @deprecated tag from FileUsageBase Constructor
Maintain the current BC layer and trigger an error if $config_factory is missing
Reorder the constructor arguments of DatabaseFileUsageBackend so that $config_factory is first and required.
Provide a BC layer if the arguments are passed in the old order and trigger an error.
Provide legacy tests to test the error triggering and that the BC layers work.
File a CR on the changing parameter order of DatabaseFileUsageBackend
File a follow-up against drupal 9 to restore type-hints and required status to parameters of both classes

Remaining tasks

review
commit

User interface changes

none

API changes

The signature of DatabaseFileUsageBackend has changed. A BC layer has been provided.

Data model changes

none

Release notes snippet

The constructor of DatabaseFileUsageBackend has had the order of its parameters changed.

CommentFileSizeAuthor
#18 interdiff.3069046.13-17.txt3.61 KBmikelutz
#18 3069046-17.drupal.Properly-manage-newly-required-parameter-of-FileUsageBaseconstruct.patch8.61 KBmikelutz
#17 interdiff.3069046.13-17.txt3.64 KBmikelutz
#17 3069046-17.drupal.Properly-manage-newly-required-parameter-of-FileUsageBaseconstruct.patch8.65 KBmikelutz
#13 interdiff.3069046.10-13.txt9.04 KBmikelutz
#13 3069046-13.drupal.Properly-manage-newly-required-parameter-of-FileUsageBaseconstruct.patch8.18 KBmikelutz
#10 interdiff_6-10.txt1.58 KBinit90
#10 3069046-10.patch1.87 KBinit90
#6 3069046-6.patch1.14 KBimalabya
#2 3069046-2.patch1.05 KBvoleger
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikelutz created an issue. See original summary.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
CreditAttribution: voleger as a volunteer and at GOLEMS GABB, Drupal Ukraine Community for Drupal Ukraine Community, GOLEMS GABB commented
Status: Active » Needs review
FileSize
3069046-2.patch1.05 KB

Added trigger_error with the message. Removed related rows from the docblock.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
CreditAttribution: voleger as a volunteer and at GOLEMS GABB, Drupal Ukraine Community for Drupal Ukraine Community, GOLEMS GABB commented
Related issues: +#2801777: Give users the option to prevent drupal from automatically marking unused files as temporary
mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
Status: Needs review » Needs work
Issue tags: +Needs tests 
  1. +++ b/core/modules/file/src/FileUsage/FileUsageBase.php
@@ -23,12 +23,13 @@
+      @trigger_error('The $config_factory parameter is deprecated in drupal:8.4.0 and become required in drupal:9.0.0. See https://www.drupal.org/project/drupal/issues/2801777', E_USER_DEPRECATED);

    nit: Calling FileUsageBase::__construct() without the $config_factory parameter is deprecated in drupal:8.4.0 and the parameter will be required in drupal:9.0.0.......

  2. We also need a test in FileLegacyTest showing that the error is triggered if the method is called without the new parameter.
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Calling FileUsageBase::__construct()

I bet here better use __METHOD__ to get full namespace

imalabya’s picture

imalabya
he/him
Primary language English
Location Bangalore
CreditAttribution: imalabya at Specbee commented
FileSize
3069046-6.patch1.14 KB

Added a patch to address to change the @trigger_error message. Keeping it in Needs work to add the test.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented 
+++ b/core/modules/file/src/FileUsage/FileUsageBase.php
@@ -23,12 +23,13 @@ abstract class FileUsageBase implements FileUsageInterface {
-   * @deprecated The $config_factory parameter will become required in Drupal
-   *   9.0.0.

this should be fixed (not removed)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

It can't be fixed, @deprecated doesn't work for partial deprecations/changes, only for complete deprecations of a function or class.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented

this should be fixed (not removed)

The tag removal is correct. The method is not deprecated, it's a base class constructor that is part of the API. This just needs a legacy test now.

init90’s picture

init90
Location Ukraine
CreditAttribution: init90 at ZANZARRA Drupal Agency, Drupal Ukraine Community commented
Status: Needs work » Needs review
FileSize
3069046-10.patch1.87 KB
interdiff_6-10.txt1.58 KB

I've added test, but not sure if approach is correct.

init90’s picture

init90
Location Ukraine
CreditAttribution: init90 at ZANZARRA Drupal Agency, Drupal Ukraine Community commented

Also, looks like we should deprecate empty $config_factory parameter for Drupal\file\FileUsage\DatabaseFileUsageBackend.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
Assigned: Unassigned » mikelutz

Also, looks like we should deprecate $config_factory parameter for Drupal\file\FileUsage\DatabaseFileUsageBackend.

Yeah, I hadn't looked at that, that's going to be trickier than I thought. The order of arguments in the constructor need to be changed, and that is going to be a little more complicated..

Assigning to myself to take a look at.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
Assigned: mikelutz » Unassigned
Issue summary: View changes
Issue tags: -Needs tests
FileSize
3069046-13.drupal.Properly-manage-newly-required-parameter-of-FileUsageBaseconstruct.patch8.18 KB
interdiff.3069046.10-13.txt9.04 KB

Okay, this patch changes the order of arguments of the DatabaseFileUsageBackend, which we have to do because $config_factory has to be required for proper DI . Proper errors are triggered, CR is made, follow-ups are filed, I think it looks good.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community

Great!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/file/src/FileUsage/DatabaseFileUsageBackend.php
@@ -28,18 +28,40 @@ class DatabaseFileUsageBackend extends FileUsageBase {
+    // @see https://www.drupal.org/project/drupal/issues/3070114
+    if ($config_factory instanceof Connection) {
+      @trigger_error('Passing the database connection as the first argument to ' . __METHOD__ . ' is deprecated in drupal:8.8.0 and will throw a fatal error in drupal:9.0.0. Pass the config factory first. See https://www.drupal.org/node/3070148', E_USER_DEPRECATED);
+      $temp = $table;
+      $table = $connection;
+      $connection = $config_factory;
+      $config_factory = $temp;
+      if (NULL === $table) {
+        $table = 'file_usage';
+      }

What about list($connection, $table, $config_factory) = func_get_args(); here?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented 
+++ b/core/modules/file/src/FileUsage/DatabaseFileUsageBackend.php
@@ -28,18 +28,40 @@ class DatabaseFileUsageBackend extends FileUsageBase {
+   *
+   * @todo Remove |mixed from the docblock above and properly type-hint the
+   *   constructor arguments in
+   *   https://www.drupal.org/project/drupal/issues/3070114 when the
+   *   drupal:9.0.x branch is opened.
    */
-  public function __construct(Connection $connection, $table = 'file_usage', ConfigFactoryInterface $config_factory = NULL) {
+  public function __construct($config_factory, $connection = NULL, $table = 'file_usage') {

We usually don't document what we accept for BC?

I'm not quite sure why this is even necessary? The argument order doesn't have to match the parent, we've ben adding new arguments to the end of child class constructors before and it worked just fine before?

See \Drupal\Core\Entity\Sql\SqlContentEntityStorage::__construct for an example in HEAD.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
FileSize
3069046-17.drupal.Properly-manage-newly-required-parameter-of-FileUsageBaseconstruct.patch8.65 KB
interdiff.3069046.13-17.txt3.64 KB

The reordering is necessary because $config_factory needs to be required, and it currently is after an optional parameter.

+1 to reassigning the arguments via list, though I needed to wrap func_get_args() with array_pad due to the optional parameters.

I added the |mixed typehint to make it pass codesniffer tests (it complains if the signature doesn't match the annotation) I agree it makes more sense to document the expected parameters and make the mismatch with the signature a coding standards exception, so I disable codesniffer around the constructor signature.

Doing that made me decide to restore some type safety and we now throw an InvalidArgumentException if the first parameter doesn't match either the legacy or current signature, and updated the test to check that exception, and to make sure the optional $table override gets reassigned correctly if passed.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
FileSize
3069046-17.drupal.Properly-manage-newly-required-parameter-of-FileUsageBaseconstruct.patch8.61 KB
interdiff.3069046.13-17.txt3.61 KB

TIL @codingStandardsIgnoreLine :-)

Interdiff from #13 because api and caching and scripted patch generation and laziness..

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented

edit - double post, please ignore.

catch’s picture

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

@codingStandardsIgnoreLine is handy here, TIL too :)

Back to RTBC.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed ae58cf0 and pushed to 8.8.x. Thanks!

  • larowlan committed ae58cf0 on 8.8.x 
    Issue #3069046 by mikelutz, init90, voleger, imalabya, andypost, catch,...

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

publish the change record