Deprecate $database argument in \Drupal\node\Plugin\views\argument\Vid::__construct()

Hi,

Tried to creating a patch for this issue.

Please review the patch.

Let me know if any suggestions.

Thanks.

Hi,

Creating a patch as suggested in #5 comment excepted 3rd point.

Please review the patch.

Thanks.

Addressing #5

1. Used DeprecatedServicePropertyTrait to deprecate $database property
2. Removed type hinting for $node_storage and handled retreiving of $node_storage in case !$node_storage instanceof NodeStorageInterface
3. Removed database service request from create()

Not providing an interdiff since #6 included changes already committed in #2628130: SQL error on revision export from view

Lets take #3157895: Move 'install_time' state update from \Drupal\Core\Installer\Form\SiteConfigureForm to installed_finished() as the example of how to do it. See: https://git.drupalcode.org/project/drupal/-/blob/HEAD/core/lib/Drupal/Co....

1. Could we add to the docblock:

      * Note, for BC reasons, we cannot typehint the last 2 parameters since this
      * function used to take 6 arguments, including the 'state' service as the
      * fourth parameter.
      *
      * @todo Clean this up in drupal:10.0.0.
      * @see https://www.drupal.org/node/3159456
   

   Only with the link to the followup for our class.

2. I am also missing the following check:

       if (!$node_storage instanceof NodeStorageInterface) {
         throw new \InvalidArgumentException('The fourth argument must implement \Drupal\node\NodeStorageInterface.');
       }
   

   and then the docblock must be updated with:

      * @throws \InvalidArgumentException
      *   Thrown when either the $module_installer or $country_manager parameters
      *   are not of the correct type.
   

1. Added follow-up issue for cleanup on isle Drupal 10
2. Added patch addressing #9

+++ b/core/modules/node/src/Plugin/views/argument/Vid.php
@@ -31,21 +31,35 @@ class Vid extends NumericArgument {
+   * @see https://www.drupal.org/node/3159456

This is not the correct followup. I think that the followup first needs to be created.

This is not the correct followup. I think that the followup first needs to be created.

It is in the attached patch. Already created follow-up issue, but forgot to change the link in the previous patch.

The patch does what the IS says it should do.
There is a CR for the deprecation.
There is also a followup issue for 10.0.
All code changes look good to me.
For me it is RTBC.

+++ b/core/modules/node/src/Plugin/views/argument/Vid.php
@@ -31,21 +31,35 @@ class Vid extends NumericArgument {
+   *   Thrown when the $node_storage parameter is not  of the correct type.

There is a double space between "is not" and "of the". Can be fixed on commit.

@alexpott: I can see the validity of your points in #15, but as pointed out by @Daffie in #9 those were also done in #3157895: Move 'install_time' state update from \Drupal\Core\Installer\Form\SiteConfigureForm to installed_finished(), which issue handles about basically the same thing: Removing a service from a constructor where the argument for that constructor isn't the last argument fed to the constructor.

1) Do we want to take #3157895: Move 'install_time' state update from \Drupal\Core\Installer\Form\SiteConfigureForm to installed_finished() as a kind of standard for these remove-non-last-argument-from-constructor issues? If so, we should keep the bits you asked to remove in #15.
2) If we opt for removing them, do we still need the follow-up issue: #3189241: Remove deprecation and fix constructor args for \Drupal\node\Plugin\views\argument\Vid

Thanks @alexpott for the clarification

We still need the follow-up to do the work in D10.

Seems we have that one covered with #3189241: Remove deprecation and fix constructor args for \Drupal\node\Plugin\views\argument\Vid?

and a yet-another-issue to clean up SiteConfigureForm prior to D10 and remove the comments there.

Isn't #3159456: Remove deprecation and fix constructor args for \Drupal\Core\Installer\Form\SiteConfigureForm already covering just that?

So I think we should remove the comments

Attached patch removes comments as requested in #15.

Keeping on Needs work because this patch doesn't cover:

Also we should add some form test here because we have logic in the constructor now. The reason we didn't add tests to #3157895: Move 'install_time' state update from \Drupal\Core\Installer\Form\SiteConfigureForm to installed_finished() is because SiteConfigureForm has the @internal annotation.

Took some inspiration from Drupal\Tests\book\Functional\BookTest::testBookNavigationCacheContextDeprecatedParameter() to add a quick test for this

The added deprecation test looks good to me.
All code changes look good to me.
We have a CR and a followup issue to restore stuff in 10.0.
For me it is RTBC.

I know that @alexpott did not want to have the following documentation in the docblock, but for me it should be in there:

   * Note, for BC reasons, we cannot typehint the last parameter since this
   * function used to take 5 arguments, including the 'database' service as the
   * fourth parameter.
   *
   * @todo Clean this up in drupal:10.0.0.
   * @see https://www.drupal.org/node/3189241

This needs to stay there. It's only the @throws documentation that's not needed. I should have been clearer.

I personally find it a bit "weird" to _not_ document the thrown InvalidArgumentException.

Also, I must say I'm on #TeamDaffie on keeping the DocBlock comments (#20). I think they help clarifying the why of not being able to typehint.

But again: Personal opinion, your milage may vary (and in fact be kilometres depending on where you live).

Thanks @alexpott for the explanation.

Sorry @daffie, but I'm switching teams ;)

Attached patch addresses #21

Reinserted the InvalidArgumentException as requested by @alexpott.
There is no testing added for the exception, because it is only temporary.
Addressed all points of @alexpott.
Back to RTBC.