dev/core#2316 - Symfony EventDispatcher dispatch() signature kerfuffle - try 3 #24132
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(Standard links)
|
|
Hmm, memory error still happens |
|
Interesting. DaveEventDispatcher makes those test fails go away by removing the listener optimizer introduced in 4.3+. Some possible reasons:
I'm not too concerned about the fails in the recent run - they all seem network-related and are likely either caused by the debug output I've added confusing the network response, or just a network glitch. I can easily test the former by removing the debug output. Let's do that... |
|
I just tried |
|
@MegaphoneJon no that would be a separate ticket / PR but I would check your composer version see also civicrm/civicrm-buildkit#715 |
5220624 to
308c310
Compare
72ece74 to
949358b
Compare
949358b to
53ab103
Compare
53ab103 to
a389dd5
Compare
a389dd5 to
f6ecd62
Compare
|
I've reviewed the code and I think it's best we merge this now while still early in the release cycle. |
|
Thanks for giving this a push. Fingers crossed🤞 |
The plugin show enabled. Hmm
I thought maybe it was the download package, so I rebuilt locally with distmaker. Same results. wpmaster built earlier this week works without issue. Let me know if we want a new issue as well on this. |
|
Ok I'll take a look. |
|
I don't think it's this PR because I can reproduce it with 5.56. I know you said it worked last week but I also tried it at http://demo-108-2i2k2.test-3.civicrm.org:8001 which is master and it works ok there, so it maybe depends on how the site is built. I can "fix" it with this patch but that's a giant sledgehammer and it would be better to figure out what is clearing the classloader. There's a lot of clearing going on - it rebuilds it 3 times but then somehow it doesn't contain afform prefixes at the time it calls runHooks. --- a/CRM/Utils/Hook/WordPress.php
+++ b/CRM/Utils/Hook/WordPress.php
$this->buildModuleList();
// Call runHooks the same way Drupal does
+ \CRM_Extension_System::singleton()->getClassLoader()->refresh();
$moduleResult = $this->runHooks(
$this->allModules,
$fnSuffix, |
|
@demeritcowboy does this help? #24962 |
|
@demeritcowboy I'll rebuild 5.56 (RC) now and see |
|
OK - I can reproduce in a new 5.55.1 install as well - so it's 100% not this. Maybe I did an upgrade on master last week - sorry about taking us down the wrong path |
|
@kcristiano can you try with #24962? |
|
I just tried the patch in #24962 - works to solve the install. |
|
@colemanw @demeritcowboy Can you also test 5.55.1 - I get a fatal when enabling diff --git a/ext/afform/core/Civi/Api4/Subscriber/AutocompleteSubscriber.php b/ext/afform/core/Civi/Api4/Subscriber/AutocompleteSubscriber.php
index 685a99c95..9f88e3cb2 100644
--- a/ext/afform/core/Civi/Api4/Subscriber/AutocompleteSubscriber.php
+++ b/ext/afform/core/Civi/Api4/Subscriber/AutocompleteSubscriber.php
@@ -11,6 +11,7 @@
namespace Civi\Api4\Subscriber;
+use Civi\Core\Service\AutoService;
use Civi\Afform\FormDataModel;
use Civi\API\Events;
use Civi\Api4\Afform;
@@ -18,8 +19,10 @@ use Symfony\Component\EventDispatcher\EventSubscriberInterface;
/**
* Preprocess api autocomplete requests
+ * @service
+ * @internal
*/
-class AutocompleteSubscriber implements EventSubscriberInterface {
+class AfformAutocompleteSubscriber extends AutoService implements EventSubscriberInterface {
/**
* @return array
@@ -34,7 +37,7 @@ class AutocompleteSubscriber implements EventSubscriberInterface {
* @param \Civi\API\Event\PrepareEvent $event
* API preparation event.
*/
- public function onApiPrepare(\Civi\API\Event\PrepareEvent $event) {
+ public function onApiPrepare(\Civi\API\Event\PrepareEvent $event): void {
$apiRequest = $event->getApiRequest();
if (is_object($apiRequest) && is_a($apiRequest, 'Civi\Api4\Generic\AutocompleteAction')) {
$formName = $apiRequest->getFormName();
diff --git a/ext/afform/core/afform.php b/ext/afform/core/afform.php
index 9aae5d998..7f5473923 100644
--- a/ext/afform/core/afform.php
+++ b/ext/afform/core/afform.php
@@ -56,7 +56,6 @@ function afform_civicrm_config(&$config) {
$dispatcher->addListener('hook_civicrm_alterAngular', ['\Civi\Afform\AfformMetadataInjector', 'preprocess']);
$dispatcher->addListener('hook_civicrm_check', ['\Civi\Afform\StatusChecks', 'hook_civicrm_check']);
$dispatcher->addListener('civi.afform.get', ['\Civi\Api4\Action\Afform\Get', 'getCustomGroupBlocks']);
- $dispatcher->addSubscriber(new \Civi\Api4\Subscriber\AutocompleteSubscriber());
// Register support for email tokens
if (CRM_Extension_System::singleton()->getMapper()->isActiveModule('authx')) {
`` |
|
Not sure if this is related, but upgrading from 5.54.0 directly to 5.55.2 gives me this fatal error:
But with the same site, upgrading from 5.54.0 (or could have been 5.54.1). to 5.55.0 worked. |
|
@herbdool This PR is only in 5.57. Do you have a stack trace from the fail? |
|
@demeritcowboy I meant to post this on the backport to 5.55. |
|
Ah ok for #24964. Cool. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
https://lab.civicrm.org/dev/core/-/issues/2316
This is a reworking of #19960 to handle the change in the signature of EventDispatcher::dispatch() across symfony v4/5/6.
Before
Calls to dispatch() all give warnings in drupal 9 because symfony reversed the order of arguments. Then they fail in drupal 10. Also, the Event class disappears in symfony 6, so can no longer reference it.
It's difficult to see these warnings in drupal 9 because it annoyingly uses
@trigger_errorwith the@, which silences the warning. But you can see them in drupal's own test environment - see e.g. this run.After
No notices. Works in drupal 10.
Technical Details
Comments
Most of the actual change is in the first 2 commits.
The last commit is because in symfony 4.3 they introduced some fancy optimization code. Something about the test runs makes it cause memory corruption and a segfault. Using a dummy subclass skips the optimization. While it would be nice to track the issue down,