Problem/Motivation

In two places, the scaffold plugin does this:

    // Call any pre-scaffold scripts that may be defined.
    $dispatcher = new EventDispatcher($this->composer, $this->io);
    $dispatcher->dispatch(self::PRE_DRUPAL_SCAFFOLD_CMD);

For a script event, dispatchScript() should be called, not dispatch().

An event triggered with dispatch() gets the wrong Event class passed to it as a parameter, and in particular, the IO object is not available.

Steps to reproduce

1. Create a plugin class with the following code

<?php

namespace YourNamespace\Composer\Plugin\YourPlugin;

use Composer\Composer;
use Composer\EventDispatcher\Event;
use Composer\EventDispatcher\EventSubscriberInterface;
use Composer\IO\IOInterface;
use Composer\Script\Event;
use Composer\Plugin\PluginInterface;

/**
 * Composer plugin for handling drupal scaffold.
 *
 * @internal
 */
class Plugin implements PluginInterface, EventSubscriberInterface {
  
  /**
   * {@inheritdoc}
   */
  public function activate(Composer $composer, IOInterface $io) {
  }

  /**
   * {@inheritdoc}
   */
  public function deactivate(Composer $composer, IOInterface $io) {
  }

  /**
   * {@inheritdoc}
   */
  public function uninstall(Composer $composer, IOInterface $io) {
  }

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents() {
    return [
      'pre-drupal-scaffold-cmd' => 'preDrupalScaffoldCmd',
      'post-drupal-scaffold-cmd' => 'postDrupalScaffoldCmd',
    ];
  }

  public static function preDrupalScaffoldCmd(Event $event) {
    $event->getIO()->write('Hello preDrupalScaffoldCmd');
  }

  public static function postDrupalScaffoldCmd(Event $event) {
    $event->getIO()->write('Hello postDrupalScaffoldCmd');
  }

}

2. Create a package with the following `composer.json` and place the file with Plugin class into `src` directory and provide references:


{
    "name": "yournamespace/yourplugin",
    "type": "composer-plugin",
    "require": {
        "php": ">=8.2",
        "composer-plugin-api": "^2"
    },
    "autoload": {
        "psr-4": {
            "YourNamespace\\Composer\\Plugin\\YourPlugin\\": "src"
        }
    },
    "extra": {
        "class": "YourNamespace\\Composer\\Plugin\\YourPlugin\\Plugin"
    }
}

3. Create a consumer website package:
composer create-project drupal-composer/drupal-project:10.x-dev some-dir --no-interaction

4. Add your custom package as a dependency to the newly created project.

5. Run `composer drupal:scaffold`

6. Observe that plugin's listeners do not have access to the `getIO()` event.

Proposed resolution

Replace the $dispatcher->dispatch(self::PRE_DRUPAL_SCAFFOLD_CMD); with $dispatcher->dispatchScript(self::PRE_DRUPAL_SCAFFOLD_CMD);.
Do the same for $dispatcher->dispatch(self::POST_DRUPAL_SCAFFOLD_CMD);.

This will allow to have access to IO and other event methods from the event handler.

Remaining tasks

  1. Add code change
  2. Add tests

User interface changes

None

API changes

The event object passed to the event handler will be an instance of class Composer\Script\Event.

Data model changes

None

Release notes snippet

Scaffold event handlers receive event object as an instance of class Composer\Script\Event.

Issue fork drupal-3266160

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

joachim created an issue. See original summary.

beatrizrodrigues’s picture

Assigned: Unassigned » beatrizrodrigues

beatrizrodrigues’s picture

Assigned: beatrizrodrigues » Unassigned
Status: Active » Needs review
joachim’s picture

Status: Needs review » Reviewed & tested by the community

Looks great. Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +Needs issue summary update

I think it is up to us whether this is a script event or not. I think we need more justification for the change. The current code is not wrong. And there's no change to tests to prove that we've fixed anything.

joachim’s picture

> I think we need more justification for the change

Subscribers to the event don't have access to IO.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alex.skrypnyk’s picture

To expand on the above comments: the `Event` passed depends on how the callback was called. If it was called from the `scripts` section of the `composer.json` - it will be of a type `Composer\EventDispatcher\Event` (the most basic event type). If it was called from the plugin - it will be of a type `Composer\Script\Event`, which has access to IO and Composer.

So the passing of the event itself works correctly. I do not see how the ask to change it to `dispatchScript()` can be fulfilled.

Note that there is a different issue related to this block of code: https://www.drupal.org/project/drupal/issues/3438034

alex.skrypnyk’s picture

I've done extensive experimenting on using Drupal Core Scaffold to distribute additional files from my project skeleton (what the Scaffold package promotes).

The requested change would indeed provide additional functionality described in https://www.drupal.org/project/drupal/issues/3182391#comment-15540532

That issue, if the approach from my comment is confirmed by the core maintainers, will become a duplicate of this one.

Note that this issue's MR does not have tests yet. I suggest to wait for https://www.drupal.org/project/drupal/issues/3438034 to get merged as it provides test fixtures for the subscriber plugins and we can then add more tests for the change suggested here.

I will be happy to pick this one up once the core maintainers approve the approach.

alex.skrypnyk’s picture

This is quite important as it will allow to access the composer object and modify scaffold items as needed.

For example, skeletons may be able to exclude their own `require` sections from the dependency pool in cases were those `require` items are used to initially create the project.

Another example is packages to be able to provide scaffold mappings for other packages.

I've described this in more detail in https://www.drupal.org/project/drupal/issues/3182391#comment-15540532

@joachim
Do you mind if I take this one on?

@alexpott
Will you accept an MR with this change and tests?

alexpott’s picture

@alex.skrypnyk probably - what you are saying makes sense but we can't pre-commit to accepting an MR before we've had a chance to review it and consider it in detail.

tannguyenhn made their first commit to this issue’s fork.

tannguyenhn’s picture

StatusFileSize
new2.4 KB

This patch for 11.x

alex.skrypnyk changed the visibility of the branch 3266160-composer-scaffold-plugin-with-tests to hidden.

alex.skrypnyk’s picture

Status: Needs work » Needs review
alex.skrypnyk’s picture

Issue tags: -Needs tests
alex.skrypnyk’s picture

smustgrave’s picture

Status: Needs review » Needs work

Haven't reviewed but issue summary update still appears to be needed.

alex.skrypnyk’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
smustgrave’s picture

Status: Needs review » Needs work

Ran test-only feature and it passed when I would expect it to fail https://git.drupalcode.org/issue/drupal-3266160/-/jobs/1375079

smustgrave’s picture

Issue tags: +Needs tests

Adding back tests tag from @alexpott in #6

larowlan’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

The test only pipeline isn't working here because there's no changes to *Test.php files so we need to run locally to verify, I'll do that.

@alex.skrypnyk - which test are you expecting to fail with the changes to the fixture?

The changes look good to me, the new event dispatched (\Composer\Script\Event) is a subclass of the old event (\Composer\EventDispatcher\Event) so there should be no BC breaks here, just more power to event subscribers - as mentioned in the issue summary - e.g. the ability to use IO.

smustgrave’s picture

Tried opening a test-only branch but can't tell if anything is failing because of the change or if HEAD is broken :(

alex.skrypnyk’s picture

@smustgrave
Thank you. Really appreciate your help here.

HEAD is failing because of https://www.drupal.org/project/drupal/issues/3417066

Hoping it will be fixed soon.

alex.skrypnyk’s picture

@larowlan

The test only pipeline isn't working here because there's no changes to *Test.php files so we need to run locally to verify, I'll do that.

This is what I was trying to explain to @smustgrave in Slack, but failed since I do not understand fully how GitLab tests these test-only VS full test (I do understand @smustgrave's point that a test need to be proving that it would test an actual change and really appreciate that he explained all that to me).

It looks like that would not work as per your comment.

So, just going to wait for the HEAD of 11.x to become unblocked.

larowlan’s picture

Yeah,.we just need to know which test fails so someone can run it locally - thanks!

smustgrave’s picture

So does appear to be failing Unit composer test with just the fixture change. So maybe that counts?
@larowlan what do you think?

larowlan’s picture

Oh, a second PR with test only changes? Thanks @smustgrave - nice one

smustgrave’s picture

So think that should be enough? Sorry @alex.skrypnyk didn't think of that approach before.

alex.skrypnyk’s picture

Status: Needs review » Reviewed & tested by the community

Ok, seeing that the updated MR passed testing and that the test-only PR has failed. Also tested this locally and all looks good.

Marking RTBC.

  • alexpott committed d3adef26 on 10.3.x
    Issue #3266160 by smustgrave, alex.skrypnyk, tannguyenhn,...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed a61eee36d2 to 11.x and 7201816d30 to 11.0.x and b676d86b91 to 10.4.x and d3adef268d to 10.3.x. Thanks!

  • alexpott committed b676d86b on 10.4.x
    Issue #3266160 by smustgrave, alex.skrypnyk, tannguyenhn,...

  • alexpott committed 7201816d on 11.0.x
    Issue #3266160 by smustgrave, alex.skrypnyk, tannguyenhn,...

  • alexpott committed a61eee36 on 11.x
    Issue #3266160 by smustgrave, alex.skrypnyk, tannguyenhn,...

Status: Fixed » Closed (fixed)

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