Problem/Motivation

The static class mapper preAutoloadDump use a hardcoded path to composer vendor such as 

$autoload['classmap'] = array_merge($autoload['classmap'], array(
        'vendor/symfony/http-foundation/Request.php',
        'vendor/symfony/http-foundation/ParameterBag.php',
        ....
));

This prevents moving vendor to a different location outside of the webroot.

Proposed resolution

It must use 

    // The current working directory for composer scripts is where you run
    // composer from.
    $vendor_dir = $event->getComposer()->getConfig()->get('vendor-dir');
    ....

$autoload['classmap'] = array_merge($autoload['classmap'], array(
        $vendor_dir.'/symfony/http-foundation/Request.php',
        $vendor_dir.'/symfony/http-foundation/ParameterBag.php',
));

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#18 interdiff.txt687 bytesMile23
#18 2764267_17.patch2.62 KBMile23
#10 preautoloaddump_use-2764267-10.patch2.67 KByogeshmpawar
#6 2764267-6.patch2.68 KBalexpott
#6 1-6-interdiff.txt2.27 KBalexpott
composer-vendor-config.patch2.61 KBwengerk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sudei created an issue. See original summary.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Component: bootstrap system » base system
Issue tags: -Composer::preAutoloadDump +Composer

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Priority: Major » Normal
Issue tags: +Needs issue summary update

Discussed with @catch, @xjm, @effulgentsia, @cilefen, and @Cottser. We decided that this is a just a normal bug since it does not affect the vast majority of users. Yes it would be nice to support multiple vendor locations but this does not appear to qualify as a major bug. See https://www.drupal.org/core/issue-priority for definition of issue priority values. @wengerk please update the issue summary with a justification if you feel the major status is warranted.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented 
    // This is, essentially, a null constraint. We only care whether the package
    // is present in vendor/ yet, but findPackage() requires it.

This comment in the same file needs to be updated.

The only other hardcoded access to vendor is in \Drupal\system\Tests\System\HtaccessTest which seems completely fine to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update
FileSize
1-6-interdiff.txt2.27 KB
2764267-6.patch2.68 KB

Fixed coding standards for concatenating with . and #5. Given my changes are cosmetic I think it is okay for me to rtbc.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2764267-6.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented
Status: Needs work » Needs review

triggered re-test

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location 🇮🇳 Pune(MH)
CreditAttribution: yogeshmpawar at QED42 commented
FileSize
preautoloaddump_use-2764267-10.patch2.67 KB

Straight re-roll for patch #6 because it's failed to apply on 8.4.x branch

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: preAutoloadDump use config composer instead of hardcoded vendor classmap » preAutoloadDump use config composer to determine vendor directory instead of hardcoding
mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH commented
Status: Needs review » Reviewed & tested by the community

Was RTBC earlier and the re-roll works well in our deployments.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: preautoloaddump_use-2764267-10.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Reviewed & tested by the community

Patch still applies to 8.4.x, and the failing test was Drupal\Tests\inline_form_errors\FunctionalJavascript\FormErrorHandlerCKEditorTest which has a sporadic fail sometimes.

Re-running test, resetting to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: preautoloaddump_use-2764267-10.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Reviewed & tested by the community

Patch still applies, re-running test, resetting RTBC (again).

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -73,32 +73,36 @@ class Composer {
+    // The current working directory for composer scripts is where you run
+    // composer from.
+    $vendor_dir = $event->getComposer()->getConfig()->get('vendor-dir');

This comment is disorienting. Would it be clearer to comment "Get the configured vendor directory."?

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
2764267_17.patch2.62 KB
interdiff.txt687 bytes

Ask and ye shall receive.

Also this patch might be good for 8.3.x as well, for better happiness with composer interactions sooner. The patch applies, so I'm leaving this as 8.4.x for the issue, but adding a test for 8.3.x.

And also: This might be a dupe for #2578485: Composer::preAutoloadDump fails with no specified classmap

Status: Needs review » Needs work

The last submitted patch, 18: 2764267_17.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review

Looks like someone re-started the 8.4.x test and it passed.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

  • larowlan committed 9eec58d on 8.4.x 
    Issue #2764267 by alexpott, Mile23, wengerk, Yogesh Pawar:...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed as 9eec58d and pushed to 8.4.x.

Thanks.

Status: Fixed » Closed (fixed)

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