-
-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REF] Require latest version of civicrm-asset-plugin #49
Conversation
Maybe I need to do something in a different order but upgrading a pretty up-to-date D8/master site after making this change I get:
|
@demeritcowboy can you make sure to a composer update on the composer-compile-plugin as well? |
Yes thanks that was eventually what I did. So maybe what this shows it that if it needs to happen in that order then people will need to know that unless there's a way to enforce order. I usually update in one go with one command so I dunno the order there might be semi-random. Or for example because composer often runs out memory some people break it into smaller steps, and might update one thing before another. Or, by now, people maybe just are used to having to fiddle every time and will eventually figure it out. |
I just got the same error updating a git-based drupal 7 using git pull and then running composer install. It updates the plugin, but then I guess still uses the older version to do the compile? Does that make sense?
Then if I just run composer install a second time it's ok. |
@demeritcowboy Aaah, that's interesting, and I think it makes a lot of sense. Some related bits of info:
I don't really know what we can do within composer v1, except try to keep the list of listeners the same in each version... |
Don't want to hijack this thread, but just another thought on that - I wonder if we might mitigate the plugin-upgrade by arranging the plugin code to reduce the odds that the listener-list cahnges in the future? To wit: instead of registering listeners as-needed, we could pre-register event-listeners with stub functions: return [
ScriptEvents::PRE_INSTALL_CMD => ['onPreInstallCmd', ...],
ScriptEvents::PRE_UPDATE_CMD => ['onPreUpdateCmd', ...],
ScriptEvents::POST_INSTALL_CMD => ['onPostInstallCmd', ...],
ScriptEvents::POST_UPDATE_CMD => ['onPostUpdateCmd', ...],
PackageEvents::PRE_PACKAGE_INSTALL => ['onPrePackageInstall', ...]
PackageEvents::POST_PACKAGE_INSTALL => ['onPostPackageInstall', ...]
...
];
}
public function onPreInstallCmd($e) { $this->validateTasks($e); }
public function onPreUpdateCmd($e) { $this->validateTasks($e); }
public function onPrePackageInstall($e) { /*stub; nothing to do*/ } The stub functions would hang around in all versions, and any iterations should be internal to those stubs. Of course, in the near-term it could provoke the problem it aims to mitigate, so not worth it for live/1.0+ plugin. But |
I think what this highlights is that we don't have a good control/measure on the interplay between As it is, I'm not really sure there is a simple+representative way to test this pre-merge. I mean, we can mock-up a build with some So, for the bigger picture, I've opened: https://lab.civicrm.org/dev/drupal/-/issues/142 For the near-term, given that it's hard to test composer-distribution pre-merge....I'm inclined to go ahead with merging. I mean, this looks reasonable and fits everyone's intuition. If there are further kinks, they'll hopefully appear during RC... |
@seamuslee001 JFYI, it occurred to me that there is a problematic side-effect -- older D8 deployments (based on Of course, we should try to keep this change, so I've posted an update to make them coexist better: https://lab.civicrm.org/dev/civicrm-asset-plugin/-/merge_requests/6 |
ping @totten @MikeyMJCO