Problem

  1. RegisterParamConvertersPass does not use the service_collector concept, because ParamConverterManager is ContainerAware.

  2. There doesn't appear to be a good reason for why it requires a full container to operate. It's reasonable to expect that there are only a handful of param converter handlers.

CommentFileSizeAuthor
#16 param_converter-2250239-16.patch16.79 KBdawehner
#9 interdiff.txt8.74 KBdawehner
#9 2250239-9.patch16.73 KBdawehner
#5 interdiff.txt4.96 KBdawehner
#5 2250239-5.patch14.23 KBdawehner
#2 2250239-2.patch11.23 KBdawehner

Comments

dawehner’s picture

dawehner
Primary language German
commented

The problem is basically the same as #2250243-1: Remove needless ContainerAware dependency from AuthenticationManager ... we don't need to lazyload the service but should just need to pass along the service ID

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2250239-2.patch11.23 KB

A first basic try.

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review

.

Status: Needs review » Needs work

The last submitted patch, 2: 2250239-2.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new 2250239-5.patch14.23 KB
new interdiff.txt4.96 KB

.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I'm fine with optionally trying to map an additional "id" parameter to the service ID.

However, can we keep the instance always as the first argument for consistency + documentation purposes, please?

       $interface = $params[0]->getClass();
...
-  public function addConverter($id, ParamConverterInterface $param_converter, $priority = 0) {
+  public function addConverter(ParamConverterInterface $param_converter, $id, $priority = 0) {

Since you're removing the runtime priority sorting, I guess we want to remove the $priority parameter?

dawehner’s picture

dawehner
Primary language German
commented

Thank you.

However, can we keep the instance always as the first argument for consistency + documentation purposes, please?

I am fine with both. Do you want the code to not figure that out automatically, or just change the existing examples?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I think we should hard-code the interface to always be parameter 0, and for the remaining optional parameters 1 and 2, I assume we need some code similar to the current patch...

Since $priority is typically optional, it would probably make sense to say that it always comes last?

void addHandler(Interface $instance [, ...] [, $priority = 0]);
dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2250239-9.patch16.73 KB
new interdiff.txt8.74 KB

Made the suggestion change.

dawehner’s picture

dawehner
Primary language German
commented

9: 2250239-9.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
commented

Anything is blocking this?

Crell’s picture

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

I can't say I'm sorry to see a ContainerAware usage go away...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2250239-9.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

9: 2250239-9.patch queued for re-testing.

The last submitted patch, 9: 2250239-9.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new param_converter-2250239-16.patch16.79 KB

3way merge

Crell’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: param_converter-2250239-16.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +Needs reroll

.

martin107’s picture

martin107 commented
Status: Needs work » Needs review

16: param_converter-2250239-16.patch queued for re-testing.

I am not certain this needs a reroll

1) The patch still applies...
2) Looking at an extract from https://qa.drupal.org/pifr/test/794243 it looks like testbot just fell over.

mkdir(): No space left on device	Warning	FileStorage.php	172	Drupal\Component\PhpStorage\FileStorage->createDirectory()
martin107’s picture

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

Now that's green again and given #17 back to RTBC

jacobsanford’s picture

jacobsanford
Primary language English
Location Fredericton 🇨🇦
commented
Issue tags: -Needs reroll

Removing 'Needs reroll' as patch applies cleanly for me. Retesting to be sure.

jacobsanford’s picture

jacobsanford
Primary language English
Location Fredericton 🇨🇦
commented

16: param_converter-2250239-16.patch queued for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 978b47c and pushed to 8.x. Thanks!

  • Commit 978b47c on 8.x by alexpott: 
    Issue #2250239 by dawehner | sun: Remove needless ContainerAware...

Status: Fixed » Closed (fixed)

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