Service tag priority does not work with HTTP Client Middleware [#3215397]

Problem/Motivation

When a priority is added to tags named 'http_client_middleware', the priority is ignored. HTTP client middlewares should use standard service_id_collector so priorities work out of the box.

Steps to reproduce

Create two HTTP middlewares, define them like so:

  test.http_middleware1:
    class: Drupal\test\HttpMiddleware1
    tags:
      - { name: http_client_middleware, priority: -100 }

  test.http_middleware2:
    class: Drupal\test\HttpMiddleware2
    tags:
      - { name: http_client_middleware, priority: 100 }

You'll see the first middleware is executed first, since it's defined first. Instead the second middleware should be executed first since priorities work in highest->lowest order.

Proposed resolution

Deprecate \Drupal\Core\DependencyInjection\Compiler\GuzzleMiddlewarePass and replace with service_id_collector.

Release notes snippet

GuzzleMiddlewarePass has been removed in favor of the generic TaggedHandlersPass, code interacting with GuzzleMiddlewarePass should use TaggedHandlersPass instead.

Comment	File	Size	Author
#6	3215397-6.patch	2.5 KB	dhirendra.mishra
#6	9.3.x: PHP 8.0 & MySQL 5.7 Patch Failed to Apply
#2	3215397-http-service-tag.patch	1.81 KB	dpi
#2	9.3.x: PHP 8.0 & MySQL 5.7 28,546 pass 9.2.x: PHP 8.0 & MySQL 5.7 28,535 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

24 May 2021 at 06:52

dpi created an issue. See original summary.

Comment #2

dpi

Perth, Australia

CreditAttribution: dpi as a volunteer and at PreviousNext commented 24 May 2021 at 10:35

Assigned:

dpi

» Unassigned

File	Size
3215397-http-service-tag.patch	1.81 KB
9.3.x: PHP 8.0 & MySQL 5.7 28,546 pass 9.2.x: PHP 8.0 & MySQL 5.7 28,535 pass

File

Size

3215397-http-service-tag.patch

1.81 KB

9.3.x:
PHP 8.0 & MySQL 5.7 28,546 pass

9.2.x:
PHP 8.0 & MySQL 5.7 28,535 pass

I'm at a loss for how to test this in a Kernel test, so posting the fix as I have it.

Comment #3

dpi

Perth, Australia

CreditAttribution: dpi as a volunteer and at PreviousNext commented 24 May 2021 at 12:03

Issue summary:

View changes

Comment #4

longwave

he/him

English

CreditAttribution: longwave at Full Fat Things commented 24 May 2021 at 14:06

Status:

Active

» Needs review

Nice find!

Pretty sure compiler passes are considered internal anyway, we can hopefully just remove the class and the reference to it in CoreServiceProvider.

Not sure we need a test either, as TaggedHandlersPassTest covers this case and we're just using it as intended here rather than a custom pass that does something similar but not as well.

As a followup it looks like ContextProvidersPass could have the same treatment, it is very similar.

Comment #5

longwave

he/him

English

CreditAttribution: longwave at Full Fat Things commented 10 July 2021 at 21:46

Status:	Needs review	» Needs work
Issue tags:	-GuzzleHttp

Remembered this following discussion of Guzzle middleware in another issue.

I think we can remove the GuzzleMiddlewarePass entirely, there is no need to keep it around.

Comment #6

dhirendra.mishra CreditAttribution: dhirendra.mishra at Valuebound for Valuebound commented 12 July 2021 at 05:54

Status:

Needs work

» Needs review

File	Size
3215397-6.patch	2.5 KB
9.3.x: PHP 8.0 & MySQL 5.7 Patch Failed to Apply

I have remove GuzzleMiddlewarePass occurance..Please review it if it hepls.

Comment #7

longwave

he/him

English

CreditAttribution: longwave at Full Fat Things commented 12 July 2021 at 07:29

Status:

Needs review

» Reviewed & tested by the community

Thanks, this looks correct to me, and I don't see the value in adding an explicit test although core committers might decide otherwise.

Comment #8

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 15 July 2021 at 03:30

I've pinged other committers about the BC implications (if any) here

Comment #9

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 15 July 2021 at 09:20

Issue tags:

+Needs change record

Discussed the BC implications with catch who felt that these would be similar to Param converters

Can we get a change notice for the removal of the class?

Thanks

We'll update the BC policy to explicitly list compiler passes when this goes in

Comment #10

longwave

he/him

English

CreditAttribution: longwave at Full Fat Things commented 15 July 2021 at 09:26

Issue tags:

-Needs change record

Added https://www.drupal.org/node/3223749

Comment #11

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 15 July 2021 at 20:24

Issue summary:	View changes
Issue tags:		+9.3.0 release notes

Saving issue credits for longwave for writing the change record 💾

Updated the BC policy

Added a release notes snippet.

Verified there's no contrib usage of this

Comment #12

15 July 2021 at 20:26

larowlan committed 02e787a on 9.3.x

Issue #3215397 by dpi, dhirendra.mishra, longwave: Service tag priority...

Comment #13

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 15 July 2021 at 20:26

Committed 02e787a and pushed to 9.3.x. Thanks!

Published the change record

Comment #14

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 15 July 2021 at 20:28

Category:	Task	» Bug report
Issue tags:		+Bug Smash Initiative

I feel this is a bug, if priorities are ignored.

As a followup it looks like ContextProvidersPass could have the same treatment, it is very similar.

We could do this, however its less important there because by nature of how they work, contexts don't typical interact with each other and hence priority isn't important.

By their very nature, middlewares are expected to be reliably ordered, so that's why I've reclassified it as a bug.