Problem/Motivation

Parent: #2982674: [meta] Composer Initiative Phase 1: Add composer build support to core

The current drupal/drupal runs several Composer scripts, specifically:

  • vendorTestCodeCleanup
  • preAutoloadDump
  • ensureHtaccess
  • upgradePHPUnit

We have to figure out how to map these events to features of the legacy project template in #2982680: Add composer-ready project templates to Drupal core, and perhaps remove or modify them in drupal/drupal.

Proposed resolution

ensureHtaccess seems like it should be the responsibility of the scaffolding plugin. #2982684: Add a composer scaffolding plugin to core

upgradePHPUnit This event was removed from 8.8.x in #3008870: Drop support for PHPUnit 4.8 once PHP 5 is no longer supported (8.8.x) so it's not needed in the legacy template.

vendorTestCodeCleanup This one is only really needed by the legacy project template, the one that assumes your entire Drupal install is underneath the server webroot. Otherwise, its safe/fine to have vendor test code if you have a proper webroot setup: #2585165: Don't include vendor test code (especially mink) in the Drupal webroot #3057094: Add Composer vendor/ hardening plugin to core

preAutoloadDump Puts some Symfony classes in the autoloader's classmap array. The (non-performance) reasons for this may be outdated. #2468499: Add vendor libs to classmap through composer script event #2253593: Stop classloader searching filesystem for classes before drupal_classloader() is called If we still need those classes in the classmap before Drupal does classloading magic, then it should be in drupal/core.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 interdiff.txt818 bytesmile23
#12 2990257_12.patch3.53 KBmile23
#10 2990257_10.patch2.73 KBmile23
#9 2990257_9.patch52.48 KBmile23

Comments

Mixologic created an issue. See original summary.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented

Also, one thing thats important to note is that upgradePHPUnit doesnt need to exist *AT ALL* because of #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7

Believe it or not, Im walking on air, I never thought I could feel so free-ee-ee.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented

I was wrong. 8.7.x, despite not actually planning to run on 5.5/5.6, will still need to test against 5.5/5.6 because many things will need to be backported to 8.6.x. So, that'll still need to happen, we just dont need to be concerned with it in the scaffold/template section because its a dev only thing.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented

The scripts are also in drupal/core. We merely need to call them from the root composer json in the project template composer.json.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented

Turns out that the script expects to run on each install by sending an individual package name. So what we need to do is add one more entry point to the script that allows for post-create-project, and iterates through the list of deps and removes their test files.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Over on the drupal-legacy-project github, we have the following: https://github.com/drupal/drupal-project-legacy/pull/1#issuecomment-4211...

For that project:

  • We basically move Drupal\Core\Composer\Composer into this package under src/. Now it has the namespace of DrupalLegacyProject\Composer. This class has a dependency on Drupal\Component\PhpStorage\FileStorage, so we make a copy of that, too. This refactoring is incomplete... We'll need to make some decisions in core about how to move this, but it needs to be moved if we're going to make this template.
  • drupal/drupal-project-legacy composer.json now has the same event scripts as drupal/drupal. They just point to the new Composer class instead of the old one. I only moved the events and not the other ones. We'll have a conversation about all this in the core issue that moves this stuff around.

All this means that we've rebuilt the core codebase using Composer, modulo the builder's own files.

In order to do that, we have to move vendorTestCodeCleanup and ensureHtaccess to a namespace within drupal-legacy-project.

As far as core is concerned, it might look like this:

  • drupal-legacy-project will eventually be merged into core. It will become a subtree split which allows for building the 'tarball' filesystem of a Drupal site.
  • Move the Composer helper class to DrupalLegacyProject\Composer.
  • Figure out how to refactor FileStorage in the same way. We use FileStorage to generate .htaccess files, and this might or might not be needed for other Drupal filesystems, such as drupal-project.
  • Add DrupalLegacyProject as a PSR-4 namespace in the drupal/drupal composer.json file. This will allow for drupal/drupal to have the same scripts for developers when they say composer install.
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Having had a conversation with @mixologic about this, #7 is not a great idea.

The reason is: Once you use the project template to install Drupal, that project template glue code will never be updated, because it's not a dependency. And we want it to never get stale. The best way to do that is to never have it in the first place.

I'm thinking that we might consider adding another project called something like drupal/composer-scripts. This would be scoped as a plugin so that composer would prioritize it. https://getcomposer.org/doc/articles/plugins.md#using-plugins

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Related issues: +#2998829: Add a composer script method to Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup
StatusFileSize
new 2990257_9.patch52.48 KB

OK, here are our concerns:

And the solution presented here:

  • Move Drupal\Core\Composer\Composer to its own 'component,' which we might call drupal/package-utilities.
  • Bikeshed on the names of things. :-)

This patch allows the core repo to be built for dev work and the tarball. It keeps the composer script elements as-is, so this patch is either the precursor to #2998829: Add a composer script method to Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup or will be the strategy for making this work after that issue is in. Presented here for conversation...

Edit: Ewps.... Managed to commit the lock file. Sorry.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Needs review
StatusFileSize
new 2990257_10.patch2.73 KB

Same as #9 without changes to composer.lock.

Status: Needs review » Needs work

The last submitted patch, 10: 2990257_10.patch, failed testing. View results

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
StatusFileSize
new 2990257_12.patch3.53 KB
new interdiff.txt818 bytes

Updates to run-tests.sh and bootstrap.php.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented

@mile23: Okay, I think I've finally caught up to where you were in #9, by attempting to make this a component. -> allow it to be used by things like the scaffold, without core needing to be there, while at the same time keeping it where core could use it for BC.

So, the part about it being a component that I was a little hesitant on, is that components are ideally supposed to be something that has standalone value outside of the project, and this seemed fairly inside baseball, so I was hesitant.

So would it be sensible to put these composer scripts (the stuff in Composer.php) into the scaffold plugin itself?

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

The update here is that we've given drupal/drupal-scaffold responsibility for vendorTestCodeCleanup since that's only required for the legacy/kickstart package: #2982684: Add a composer scaffolding plugin to core

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

See: #3057094: Add Composer vendor/ hardening plugin to core

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Category: Task » Plan
Issue summary: View changes
Status: Needs review » Active
Related issues: +#3057094: Add Composer vendor/ hardening plugin to core, +#2982684: Add a composer scaffolding plugin to core, +#2982680: Add composer-ready project templates to Drupal core

Just to update related issues and point out that we don't need to consider upgradePHPUnit any more: #3008870: Drop support for PHPUnit 4.8 once PHP 5 is no longer supported (8.8.x)

We also discussed preAutoloadDump in the Composer Initiative meeting: #3064279: Agenda for 6-26-2019 Our general consensus was that the optimization seems vestigial, but that we'd need to figure out a way to determine whether leaving it out of the legacy template would be a regression.

Switching this to a plan issue because it's clear the patch in #12 is not at all relevant. :-)

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Issue tags: -Composer +Composer initiative
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Both vendorTestCodeCleanup and ensureHtaccess will now live in the vendor cleanup plugin, which is now called drupal/core-vendor-hardening.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Status: Active » Fixed

https://www.drupal.org/project/drupal/issues/2468499 seems to be the reason for preAutoloadDump

I think that its *very* vestigal in the fact that after this, essentially all projects will be 'composer projects'

Pretty much 100% we can get rid of it from drupal/drupal.

It gets called in drupal/core as well, so I think it may or may not still serve a purpose there. Either way if we build out the legacy template, that will already get used, so we really dont have to do anything at all with it and things will stay the same.

So, given that Im pretty confident we've got a plan for all the composer scripts that were in drupal/drupal and that we can close this issue.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

🎉

Status: Fixed » Closed (fixed)

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