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
Add code changeAdd 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.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 3266160-call-dispatch-script.patch | 2.4 KB | tannguyenhn |
Issue fork drupal-3266160
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
Comment #2
beatrizrodriguesComment #4
beatrizrodriguesComment #5
joachim commentedLooks great. Thanks!
Comment #6
alexpottI 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.
Comment #7
joachim commented> I think we need more justification for the change
Subscribers to the event don't have access to IO.
Comment #11
alex.skrypnykTo 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
Comment #12
alex.skrypnykI'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.
Comment #13
alex.skrypnykThis 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?
Comment #14
alexpott@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.
Comment #16
tannguyenhn commentedThis patch for 11.x
Comment #18
alex.skrypnykComment #19
alex.skrypnykComment #20
alex.skrypnykComment #21
smustgrave commentedHaven't reviewed but issue summary update still appears to be needed.
Comment #22
alex.skrypnykComment #23
smustgrave commentedRan test-only feature and it passed when I would expect it to fail https://git.drupalcode.org/issue/drupal-3266160/-/jobs/1375079
Comment #24
smustgrave commentedAdding back tests tag from @alexpott in #6
Comment #25
larowlanThe 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.Comment #27
smustgrave commentedTried opening a test-only branch but can't tell if anything is failing because of the change or if HEAD is broken :(
Comment #28
alex.skrypnyk@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.
Comment #29
alex.skrypnyk@larowlan
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.
Comment #30
larowlanYeah,.we just need to know which test fails so someone can run it locally - thanks!
Comment #31
smustgrave commentedSo does appear to be failing Unit composer test with just the fixture change. So maybe that counts?
@larowlan what do you think?
Comment #32
larowlanOh, a second PR with test only changes? Thanks @smustgrave - nice one
Comment #33
smustgrave commentedSo think that should be enough? Sorry @alex.skrypnyk didn't think of that approach before.
Comment #34
alex.skrypnykOk, 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.
Comment #36
alexpottCommitted 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!