Problem/Motivation

The classmap in composer is for own classes and not classes from vendor libraries. The hardcoded paths to HttpKernel etc. makes it impossible swap these classes in composer bases projects.

Proposed resolution

Add the necessary classes through composer script events. This keeps the performance improvements from #2253593: Stop classloader searching filesystem for classes before drupal_classloader() is called but also provides subtree-split compatibility.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug, because this breaks composer integration and is wrong from a composer standpoint.
Issue priority Normal, because it can be work-arounded in some ways.
Disruption None, because the autoloader is no public api and the generated autoload is exactly the same as before.
CommentFileSizeAuthor
#14 vendor-classmap-2468499-14.patch2.81 KBwebflo
#6 vendor-classmap-2468499-6.patch2.88 KBwebflo
remove-vendor-classmap.patch3.05 KBwebflo

Comments

webflo’s picture

webflo commented
Status: Needs review » Active
webflo’s picture

webflo commented
Status: Active » Needs review
webflo’s picture

webflo commented
Issue summary: View changes
moshe weitzman’s picture

moshe weitzman commented

Any idea why they were added to classmap in the first place?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

#2253593: Stop classloader searching filesystem for classes before drupal_classloader() is called

That's why. They are loaded before we can set up the configurable classloader, so we can't cache them with apc classloader.

webflo’s picture

webflo commented
StatusFileSize
new vendor-classmap-2468499-6.patch2.88 KB

Ok, new approach. We could use a composer script event to add the necessary to composers classmap. Script events are executed on root packages only. This means the script wont run if we require drupal/core as composer dependency.

webflo’s picture

webflo commented
Title: Remove vendor libs from classmap » Add vendor libs to classmap through composer script event
webflo’s picture

webflo commented
Issue summary: View changes
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

If that is comparable for performance then I guess it is fine. Probably needs feedback from @neclimdul and @Fabianx.

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

Okay, that is very great!

It means it will be pretty simple to add a full classmap, too for those that want to use a full classmap.

RTBC, thanks for the ping in the other issue Berdir.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -0,0 +1,42 @@
+    /**
+     * @var \Composer\Package\RootPackage $package
+     */

This comment is not in the correct format and can just be removed.

webflo’s picture

webflo commented
Issue summary: View changes
webflo’s picture

webflo commented
Issue summary: View changes
webflo’s picture

webflo commented
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
StatusFileSize
new vendor-classmap-2468499-14.patch2.81 KB

Updated the Beta phase evaluation and removed the comment.

fabianx’s picture

fabianx commented
Category: Task » Bug report
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Updated beta, this is a bug and not a task as composer is broken.

alexpott’s picture

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

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 21b27e3 and pushed to 8.0.x. Thanks!

  • alexpott committed 21b27e3 on 8.0.x 
    Issue #2468499 by webflo: Add vendor libs to classmap through composer...
bojanz’s picture

bojanz commented

I don't see a reply to Berdir's comment in #9.

Won't the root composer.json need to add the pre-autoload-dump command too, to ensure that it ends up with those files in the classmap as expected?

fabianx’s picture

fabianx commented

#18:

1. As the generated classmap is the same, the performance is the same.
2. I cannot speak to the composer vs. root composer part, but I suspect it is not needed as that one just references core/composer.json?

@webflo: ^^

Status: Fixed » Closed (fixed)

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