Problem/Motivation

The context.repository service uses a custom compiler pass in order to get all services tagged as context_provider passed as a constructor argument. This can be achieved in the same way by utilizing the service_id_collector tag and does not need a custom compiler pass.

Steps to reproduce

-

Proposed resolution

Use the service_id_collector pattern for the context.repository service and deprecate ContextProvidersPass.

Remaining tasks

User interface changes

-

API changes

-

Data model changes

-

Release notes snippet

-

Issue fork drupal-3394680

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

tstoeckler created an issue. See original summary.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Not sure if I'm missing anything and the compiler pass is in fact needed, but just stumbled upon this and was a bit confused. Will open an MR, let's see if it's green.

tstoeckler opened merge request !5028

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Tests appear green.

Will need a CR.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review

Fair enough, although I wasn't sure what to write in the change notice, as I can't fathom any reason why someone would be using that.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Thanks!

longwave’s picture

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

Compiler passes are considered internal, I don't think this warrants a change record as I also don't see how anyone else would be extending or reusing it, so I've deleted the change record.

Committed and pushed 0a13623646 to 11.x and d0b7905944 to 10.2.x. Thanks!

  • longwave committed d0b79059 on 10.2.x 
    Issue #3394680 by tstoeckler: Deprecate ContextProvidersPass in favor of...

  • longwave committed 0a136236 on 11.x 
    Issue #3394680 by tstoeckler: Deprecate ContextProvidersPass in favor of...
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Awesome, thanks! Opened #3396445: [11.x] Remove deprecated ContextProvidersPass for the actual removal.

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone commented
Status: Closed (fixed) » Needs work
Issue tags: +Needs change record updates

This issue added a deprecation with a link to a change record that does not exist.

I added a draft change record so the docbloc can be updated. It is not complete so I am tagging for change record updates.

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
commented

Took a stab at the change record - are there any situations in which someone might use that class elsewhere, extend it, or override it? I can't think how that would be useful let alone possible.

Also shouldn't the 11.x commit remove the class?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

I was quite confused by #13, but then re-read the issue and this is what happened:

  1. In #5 @smustgrave requested a CR after which I wrote one and updated the link in the code.
  2. In #8 @longwave considered the CR unnecessary, thus deleted it, but committed the patch with the link
  3. In #13 @quietone noticed the broken link and added a (draf) CR

So maybe you three need to hop on a call and discuss what does or does not warrant a CR ;-)
When there's consensus on whether or not one is needed, the link can either be dropped or updated. I don't feel strongly either way (as hinted at in #6 I could certainly live without one, but I don't really care).

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Also Re #14:

  1. I couldn't think of any legitimate reason why someone would extend the class either.
  2. The 11.x branch (somewhat confusingly) is not yet opened for 11.x development, but always targets the next 10.x release (i.e. 10.3.x right now). I already opened #3396445: [11.x] Remove deprecated ContextProvidersPass which can be tackled once 11.x development actually starts.
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

As we need to link the deprecation somewhere we do need a change record, that was my mistake.

longwave opened merge request !6272

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

Opened a new MR with the change record link. Apologies for wasting everyone's time here.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

CR link seems good

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record updates

Committed and pushed cc8a853306 to 11.x and ce3da88a84 to 10.2.x.

  • longwave committed ce3da88a on 10.2.x 
    Issue #3394680 followup by longwave: Deprecate ContextProvidersPass in...

  • longwave committed cc8a8533 on 11.x 
    Issue #3394680 followup by longwave: Deprecate ContextProvidersPass in...

longwave closed merge request !6272

longwave closed merge request !5028

Status: Fixed » Closed (fixed)

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