Problem

  1. When ExtensionDiscovery encounters the same extension in a later search directory, then it parses its .info file to prevent a stale/outdated extension from an earlier major version of core to mistakenly override a new core module.

    Example: Contrib image.module from D6 is still located in /sites/all/modules, but D7 ships with an image.module in core.

  2. With Migrate in core, that situation should theoretically no longer occur, because you are migrating between two completely detached code bases.

Proposed solution

  1. Remove the core compatibility check from ExtensionDiscovery.

    → The class should be 100% about discovery only.

CommentFileSizeAuthor
#14 2209293-psr4-reroll.patch3.47 KBxjm
#13 drupal8.ext-disco-core-compat.13.patch3.52 KBsun
#6 drupal8.ext-disco-core-compat.6.patch3.55 KBsun
#4 drupal8.ext-disco-core-compat.4.patch2.25 KBsun
drupal8.ext-disco-core-compat.0.patch1.2 KBsun

Comments

Status: Needs review » Needs work

The last submitted patch, drupal8.ext-disco-core-compat.0.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

The test failure is expected.

Let's get an agreement on the proposed change first.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

+1 to this clean-up

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.ext-disco-core-compat.4.patch2.25 KB

Attached patch should come back green.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Needs work 
+++ b/core/modules/system/lib/Drupal/system/Tests/Common/SystemListingTest.php
@@ -29,13 +29,6 @@ function testDirectoryPrecedence() {
-      'drupal_system_listing_incompatible_test' => array(

AFAIK this means we can actually remove those two test modules from the repo.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.ext-disco-core-compat.6.patch3.55 KB

Done so.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

Awesome thanks. Looks great

catch’s picture

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

What will happen if you have the wrong branch of a module in your site with this check removed?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

If you mean a D7 (or below) branch... then at least for 8.x, that module won't be discovered in the first place, because it doesn't have a .info.yml file.

We can re-evaluate the situation for D9 — unless D9 [hopefully] switches to composer.json, which in turn would cause the same situation as with .info.yml in D8.

catch’s picture

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

Well that's a good point. I was thinking if you did something like a git checkout and checked out the wrong branch, it'd be nice not to blow everything up.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: drupal8.ext-disco-core-compat.6.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +Needs reroll

.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
StatusFileSize
new drupal8.ext-disco-core-compat.13.patch3.52 KB
xjm’s picture

xjm
she/her
Primary language English
commented
StatusFileSize
new 2209293-psr4-reroll.patch3.47 KB

Reroll for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4.

catch’s picture

catch
he/him
Primary language English
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 commented

ExtensionDiscovery::$infoParser, and ExtensionDiscovery::getInfoParser() is now no longer needed and can be removed.
Or could have been removed int his issue.
Right?