Problem/Motivation

In #3383339: Replace implementing deprecated interface Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface with ValueResolverInterface we removed Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface from the implements of \Drupal\Core\Controller\ArgumentResolver\Psr7RequestValueResolver and
\Drupal\Core\Controller\ArgumentResolver\RouteMatchValueResolver.

This means that the supports() function in both resolvers is now cruft, since it was only there to fulfil the contract from Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface.

@catch in #3383339-10: Replace implementing deprecated interface Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface with ValueResolverInterface :

However, do we need a follow-up to deprecate and then remove methods like Psr7RequestValueResolver::supports() in 11.x? Should be cruft now.

This issue is for deprecating both supports()s.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3383548

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

Comments

Spokje created an issue. See original summary.

spokje’s picture

spokje
Primary language Dutch
commented
Issue summary: View changes

Spokje opened merge request !4657

spokje’s picture

spokje
Primary language Dutch
commented
Status: Active » Needs review

Since there's no usage of both supports() in core currently, I believe we don't need a deprecation test for this one?

mondrake’s picture

mondrake
Location 🇮🇹
commented

Being final classes, maybe we can just remove the methods?

spokje’s picture

spokje
Primary language Dutch
commented

Would love that and it makes sense, but there currently doesn't seem to be any ruling on that in https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... and I have been bitten too much by doing things not described in there to do so straight up in here.

Maybe this could be the issue to enhance that?

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

I think argument resolvers would be covered under @internal insofar as they're equivalent to plugins and controllers and therefore not explicitly named. However also I think it's easier to deprecate + remove when in doubt same as we do for constructors unless something particularly makes that difficult. final helps for removing protected methods but not for public, so don't think there's anything particularly different there.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Reviewed & tested by the community » Needs work

Second thinking, it’s not just about extending classes but also about potential direct calls to the methods in these classes. So let’s deprecate - but now with PHPStan doing the check for calls, I think we should do the @deprecate and @see dance in the docblocks.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Ironic, and absolutely right, thanks @mondrake

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

Thanks!

  • catch committed 58ce7d4e on 11.x 
    Issue #3383548 by Spokje, mondrake: Deprecate Psr7RequestValueResolver::...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 58ce7d4 and pushed to 11.x. Thanks!

Spokje closed merge request !4657

spokje’s picture

spokje
Primary language Dutch
commented

Added "Introduced in version" in CR.

Should it be published now or when 10.2.0 ships?

catch’s picture

catch
he/him
Primary language English
commented

Now, but I frequently forget. Published.

Status: Fixed » Closed (fixed)

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