Problem/Motivation

MainContentRenderersPass maps services tagged with formats to their service names and stores it in a parameter.

MainContentViewSubscriber uses this map and the class resolver to instantiate the correct renderer service.

The compiler pass and parameter could be replaced with a tagged service locator injected into MainContentViewSubscriber.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3469143

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

longwave created an issue. See original summary.

longwave opened merge request !9276

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review 
 5 files changed, 18 insertions(+), 73 deletions(-)
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

This is an event subscriber and I doubt anyone would be overriding it (you would swap out the renderer service instead) and so I don't feel we need to provide BC. We could deprecate the unused compiler pass instead of deleting it outright if that is deemed necessary.

smustgrave’s picture

smustgrave commented

Since this file wasn't marked @internal any backwards compatibility concerns with deleting?

longwave’s picture

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

Will try and add some BC.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

longwave’s picture

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

Came back to revisit this. I started to add a deprecation notice to the compiler pass, but really I don't think it's worth it. Our BC policy states that compiler passes are internal and not considered part of the API.

dcam’s picture

dcam commented

I had one minor comment on the MR. Otherwise it looks good to me.

dcam’s picture

dcam commented
Status: Needs review » Needs work

Setting to Needs Work because of the unresolved question on the MR.

longwave’s picture

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

Changed the type to ServiceLocator.

dcam’s picture

dcam commented
Status: Needs review » Needs work

Sorry, I found a documentation issue on my final review.

longwave’s picture

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

Thanks for reviewing this and many of my other MRs recently!

dcam’s picture

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

Thanks for reviewing this and many of my other MRs recently!

I am happy to help.

My feedback has been addressed. This looks good to me.

catch’s picture

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

This need a rebase. Agreed that we should not try to implement bc for either the constructor changes or the compiler pass here.

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

Rebased

  • catch committed 8d0ea1d3 on 11.x 
    task: #3469143 MainContentViewSubscriber should use a service locator...

  • catch committed 98b2f7c7 on main 
    task: #3469143 MainContentViewSubscriber should use a service locator...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !9276

Status: Fixed » Closed (fixed)

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