Problem

  1. RegisterAuthenticationPass does not use the service_collector concept because someone was lazy to convert it.

  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 authentication providers.

CommentFileSizeAuthor
#10 interdiff.txt668 bytessun
#10 authentication-2250243-10.patch4.78 KBsun
#6 interdiff.txt1.27 KBdawehner
#6 authentication-2250243-5.patch4.74 KBdawehner
#4 interdiff.txt1.1 KBdawehner
#4 authentication-2250243-3.patch4.47 KBdawehner
#2 authentication-2250243-1.patch3.37 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

... In fact we just store the service ID to not have to figure out which authentication manager applies to which route at runtime. I would love to extend #2213319: Create a single Container CompilerPass to collect + add handlers to consumer service definitions to support service IDs as well.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue summary: View changes
Status: Active » Needs review
FileSize
authentication-2250243-1.patch3.37 KB

Status: Needs review » Needs work

The last submitted patch, 2: authentication-2250243-1.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
authentication-2250243-3.patch4.47 KB
interdiff.txt1.1 KB

This time it should actually work.

Status: Needs review » Needs work

The last submitted patch, 4: authentication-2250243-3.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
authentication-2250243-5.patch4.74 KB
interdiff.txt1.27 KB

Fixed the failure.

dawehner queued 6: authentication-2250243-5.patch for re-testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

A welcome improvement.

chx’s picture

chx CreditAttribution: chx commented

this shouldn't hold the commit up but

$id = substr($id, strlen('authentication.'));

that just rubs me the wrong way. Is there any reason not to put that in a comment and instead run a strlen every time this is ran? I know microoptimization and all but still, this is ridiculous (and if I am not mistaken this runs on every request). Do we expect that the length of the string authentication. to change any time soon?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
authentication-2250243-10.patch4.78 KB
interdiff.txt668 bytes

Agreed, replaced accordingly.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

+1 for this optimization. This runs at least on potentially every request, so its somehow worth.

alexpott’s picture

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

Committed a704796 and pushed to 8.0.x. Thanks!

  • alexpott committed a704796 on 8.0.x 
    Issue #2250243 by dawehner, sun: Remove needless ContainerAware...

Status: Fixed » Closed (fixed)

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