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.

Files: 
CommentFileSizeAuthor
#11 1846376.patch3.28 KBeffulgentsia
Test request sent.
[ View ]
#4 1846376.patch3.28 KBamateescu
Test request sent.
Previous result: FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#1 interdiff.txt2.97 KBeffulgentsia

Comments

effulgentsia’s picture

StatusFileSize
new2.97 KB
catch’s picture

Status:Postponed» Needs review

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

catch’s picture

Priority:Normal» Major
amateescu’s picture

Priority:Major» Normal
StatusFileSize
new3.28 KB
Test request sent.
Previous result: FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

Let's try to test it then.

amateescu’s picture

Priority:Normal» Major

Ugh.

Status:Needs review» Needs work

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

amateescu’s picture

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

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

Status:Needs review» Needs work

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

c4rl’s picture

Status:Needs work» Needs review

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

effulgentsia’s picture

StatusFileSize
new3.28 KB
Test request sent.
[ View ]

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

#11 passed the BOT.

sun’s picture

Status:Needs review» Reviewed & tested by the community

Thanks, looks good to me.

catch’s picture

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

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

<?php
       
// 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

Issue summary:View changes

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