This is a follow-up from #1784312: Stop doing so much pre-kernel bootstrapping - the ClassLoader test that checks that the namespaces of disabled modules don't get registered had to be changed there. This was because we need to register module namespaces before accessing any service in the container, but we end up doing this right in the kernel itself and using an out of date list of modules if a module has just been disabled. Subsequent requests do not result in the namespace getting registered, but we should really try to find a way to prevent it happening even once.

CommentFileSizeAuthor
#11 1846376.patch3.28 KBeffulgentsia
#4 1846376.patch3.28 KBamateescu
#1 interdiff.txt2.97 KBeffulgentsia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
FileSize
interdiff.txt2.97 KB

This can be applied on top of #1784312-76: Stop doing so much pre-kernel bootstrapping.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Postponed » Needs review

About to commit the other one, let's get this in quick.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Normal » Major
amateescu’s picture

amateescu CreditAttribution: amateescu commented
Priority: Major » Normal
FileSize
1846376.patch3.28 KB

Let's try to test it then.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Priority: Normal » Major

Ugh.

Status: Needs review » Needs work

The last submitted patch, 1846376.patch, failed testing.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs work » Needs review

That's an unrelated test failure, HEAD is broken atm: #1842726: Transliteration component must not contain drupal_alter().

c4rl’s picture

c4rl CreditAttribution: c4rl commented

#4: 1846376.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1846376.patch, failed testing.

c4rl’s picture

c4rl CreditAttribution: c4rl commented
Status: Needs work » Needs review

#4: 1846376.patch queued for re-testing.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
FileSize
1846376.patch3.28 KB

HEAD is working again, and #4 passed the retest, but isn't turning green for some reason, so here it is again.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

#11 passed the BOT.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Thanks, looks good to me.

catch’s picture

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

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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

donquixote’s picture

donquixote CreditAttribution: donquixote commented
Status: Closed (fixed) » Needs work

Does someone in here still remember this issue?

I am pretty sure this "fix" no longer works in current Drupal 8.
At least with the Composer ClassLoader, there is no way to "unregister" a namespace. Whatever is still happening there is bogus now...

http://drupalcode.org/project/drupal.git/blob/1f9a488b927e9548365bf4882e...

        // Revert the class loader to its prior state. However,
        // registerNamespaces() performs a merge rather than replace, so to
        // effectively remove erroneous registrations, we must replace them with
        // empty arrays.
        $namespaces_after = $this->classLoader->getPrefixes();
        $namespaces_before += array_fill_keys(array_diff(array_keys($namespaces_after), array_keys($namespaces_before)), array());
        $this->registerNamespaces($namespaces_before);

And here is what ClassLoader::add() actually does..
http://drupalcode.org/project/drupal.git/blob/1f9a488b927e9548365bf4882e...

This can only ever add to the registered namespaces, never remove any.

I am keeping this mechanic in the PSR-4 patch,
#2083547: PSR-4: Putting it all together
but I would prefer to remove it.

We can open a new issue for this, but I would like to get some feedback from the old days' participants first..
(and apologies if reopening is the wrong thing to do)

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Issue summary: View changes

Since modules can only be uninstalled now, are the concerns in #16 still applicable?

jeroenmarinus’s picture

jeroenmarinus
Location Antwerp
CreditAttribution: jeroenmarinus at Mia Interactive commented
Status: Needs work » Closed (fixed)
Issue tags: +Barcelona2015

I have reviewed the comment of donquixote, but cannot find any traces of the code he references.

Just as jhedstrom mentions, it also doesn't seem to be relevant anymore. Closing this issue, if it somehow is not ok maybe it's better to open a new one.