Rules registers no listeners on rare occasions.

Setup: D10.0.0, PHP 8.1.9, typed_data 8.x-1.x-dev

I got to this issue after rebuilding the cache via drush - drush cr.
The patch #102 solves the issue for me.

drush cc router also gets to a state that rules fire.

@drupal-ramesh, be sure to apply both patches. There are two patches, one for Core and another for Rules. It looks like you may have installed the Rules patch only.

I've been using the patches at #102, which for me solve the immediate issue. However, I just noticed that they seem to break something else: after installing the patch, install.php no longer shows the nice "Drupal already installed" page but a page showing exceptions with a stacktrace.

To reproduce:
Install rules without this patch.
Go to .../core/install.php. Observe the "Drupal already installed" page.
Install patch #102. Reload .../core/install.php.

I see a page with this content:

Additional uncaught exception thrown while handling exception.
Original

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "state". in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 1030 of /...path-omitted.../dev/vendor/symfony/dependency-injection/ContainerBuilder.php).

Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition('state') (Line: 600)
Symfony\Component\DependencyInjection\ContainerBuilder->doGet('state', 1) (Line: 558)
Symfony\Component\DependencyInjection\ContainerBuilder->get('state') (Line: 488)
Drupal::state() (Line: 89)
Drupal\rules\EventSubscriber\GenericEventSubscriber::getSubscribedEvents() (Line: 37)
Drupal\Core\DependencyInjection\Compiler\RegisterEventSubscribersPass->process(Object) (Line: 94)
Symfony\Component\DependencyInjection\Compiler\Compiler->compile(Object) (Line: 762)
Symfony\Component\DependencyInjection\ContainerBuilder->compile() (Line: 1344)
Drupal\Core\DrupalKernel->compileContainer() (Line: 948)
Drupal\Core\DrupalKernel->initializeContainer() (Line: 20)
Drupal\Core\Installer\InstallerKernel->initializeContainer() (Line: 487)
Drupal\Core\DrupalKernel->boot() (Line: 426)
install_begin_request(Object, Array) (Line: 116)
install_drupal(Object) (Line: 48)

Additional

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "theme.manager". in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 1030 of /...path-omitted.../dev/vendor/symfony/dependency-injection/ContainerBuilder.php).

Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition('theme.manager') (Line: 600)
Symfony\Component\DependencyInjection\ContainerBuilder->doGet('theme.manager', 1) (Line: 558)
Symfony\Component\DependencyInjection\ContainerBuilder->get('theme.manager') (Line: 659)
Drupal::theme() (Line: 22)
_drupal_maintenance_theme() (Line: 506)
drupal_maintenance_theme() (Line: 1025)
install_display_output(Array, Array) (Line: 271)
_drupal_log_error(Array, 1) (Line: 365)
_drupal_exception_handler(Object)

Undo the patches. Reload .../core/install.php. You get the "Drupal already installed" page again.

Sorry for the noise here, was trying for yet another approach. Maybe it was even already discussed and tryed before here, didn't fully understand every detail that was raised in this issue.

Attached a patch that is using following approach:

• Excluding the GenericEventSubscriber from the general service collector, as it may be called very early where not all container services are yet available (especially the state service).
• Instead, GenericEventSubscriber::getSubscribedEvents would be called on behalf of the event dispatcher in the moment the event_dispatcher service is being instantiated by the service container.

This approach might be more reliable, but with the cost of performance as it would read from the state key-value table everytime the event_dispatcher is being instantiated. Might be interesting to see whether that is noticeable.

I tried using the decorator pattern first on the event_dispatcher service, but unfortunately then the Drupal\Core\DependencyInjection\Compiler\RegisterEventSubscribersPass does not work anymore (maybe because of decorating a service, its definition is not yet available on that point, could be a core bug). So the only way I saw was to override the service definition itself.

Haven't thoroughly tested this approach, was just curious whether it may be a way without the need to patch core itself.

The last submitted patch, 131: 2816033-event-dispatcher-131.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Setting back to NR in favor of #102.

Adjusted patch from 131 a bit so that the event dispatcher can be refreshed on runtime.

Thanks for looking at this. I haven't sat down to try out your patch yet, but I'm really interested in seeing new approaches to the problem.

I don't know if I've explicitly said it before, but the problem as I see it is that in order to allow for configurable rules, Rules needs to be able to dynamically change event subscribers. But Symfony/Drupal only builds the list of event subscribers once, and that happens when the container is built. The architecture of Symfony/Drupal does not currently permit event subscribers to be changed dynamically, so what Rules has to do here is work-around this limitation in Symfony/Drupal. And the work-around we use doesn't always work.

What Rules does now is, when we change event subscribers (for example when we add/modify/delete a rule) we force a container rebuild in order to force Symfony/Drupal to recalculate its list of event subscribers.

The reason this sometimes fails is that during the container rebuild, when Symfony/Drupal is calculating what the event subscribers are, we can't guarantee that ANY service defined in the container is available yet - because the container is incomplete at this point. So in public static function getSubscribedEvents(), we can't currently count on using ANY service because it may not work.

My hack in #102 adds a core change to pass the "old" container into getSubscribedEvents() so that we can use the services of the "old" container while Symfony/Drupal is in the middle of building the new container. This IS guaranteed to work, but it requires a change to core.

We need a way to persist the list of events that Rules uses across container rebuilds, so that when Symfony/Drupal builds a new container we can tell it which events we want to subscribe to. This is not a static list that we can hardwire into some static getSubscribedEvents() method - it is a dynamic list that we have to determine on-the-fly based on the existing Rules configurations. No matter how you store this dynamic list, via the state service like we currently use, or explicitly in the DB, or on the file system, or in configuration, we need to use a service in getSubscribedEvents() to retrieve this list. And that's the problem.

we need to use a service in getSubscribedEvents() to retrieve this list. And that's the problem.

Looks like this may be a circumstance that wasn't enough considered when the performance optimization was implemented within Drupal core regards the event dispatcher. As we cannot rely on service availability in the moment that method is being currently invoked, we might need to find a different point where the services are available.

I've just recognized that the event dispatcher service allows to add an event subscriber on runtime. So I tried another approach, using a lazy event subscriber. This might be better than my initial approach from #134, because that doesn't require the event dispatcher service to be replaced by a Rules-specific implementation. Attached a patch for the lazy subscriber approach. For that approach the arguments mentioned in #131 regards performance and container rebuilds remain the same.

The last submitted patch, 136: 2816033-lazy-subscriber-136.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Setting back to NR in favor of #102 and for manual review of #134 and #136.

Patch in #136 fixes the issue for me on drush cr. Checked the failing tests on the patch and they are due to the logger re-addition not being needed anymore, and generating duplicated logs:

    // The logger instance has changed, refresh it.
    $this->logger = $this->container->get('logger.channel.rules_debug');
    $this->logger->addLogger($this->debugLog);

probably a result of

A benefit of this approach is that there is no more need to trigger a full rebuild of the service container once an event has been added or deleted within a reaction rule config.

Attached patch remove this re-addition in all kernel tests as it seems not needed anymore.

(..) but I'm really interested in seeing new approaches to the problem.

Well in that case, find attached a patch with yet another approach to choose from. It's a simple workaround without patching core:

• Container might not be available but Database is.
• state service is just a wrapper around key_value service, and this has a very simple storage structure so query directly the key_value database table.

Actually it is the one used by Business Rules → (see their repo).

The last submitted patch, 140: 2816033-use-database-connection-140.patch, failed testing. View results →

Attached patch remove this re-addition in all kernel tests as it seems not needed anymore.

Makes sense, thanks for fixing the tests.
I currently prefer the patch #139. If that approach may be viable for others too, we could also consider removing the state usage and instead make use of cache.backend.chainedfast that may hold the list of events, read out from the Rules configurations. That may further optimize the runtime performance a bit.

Container might not be available but Database is.

That's actually not guaranteed either, despite the fact that the database service may be more likely available.

state service is just a wrapper around key_value service, and this has a very simple storage structure so query directly the key_value database table.

That's true for the default (core) implementation of the state service. But in theory, the service could be replaced or decorated such that it behaves differently, such as not using the database table and instead using a separate key-value database. If I recall correctly, this is what you can do with the Redis → module.

Setting back to NR for 139/102/140.

Sorry, I haven't had a chance to look into #136 in detail yet. I've been away a lot.

Re: #139

Checked the failing tests on the patch and they are due to the logger re-addition not being needed anymore, and generating duplicated logs

Thank you for figuring this out, this is a very good thing! I have never like that code in the Kernel tests - it was a hack needed to keep the tests running. It's good to know that was needed as a direct result of what is done here in the event subscriber, and it will be good to get rid of that stuff.

Re: #140

Container might not be available but Database is.
state service is just a wrapper around key_value service, and this has a very simple storage structure so query directly the key_value database table.

I looked at the code in that other module and agree with @mxh that building direct SQL calls like that other module does is not a good idea for a variety of reasons. We take great care in Drupal to abstract our storage, and putting explicit SQL into this important method sort of defeats all the rest of the architecture in Drupal. Your patch in #140 is better because it uses the Drupal database API rather that using SQL, but we should still try to avoid that IMO. The database service would be even better, but the problem we have here is that the availability of any service can't be guaranteed at this point in the container build (this is something I did try already instead of the state service).

However, as a quick fix I think #140 could be used in the near term to avoid this problem, if we can't figure out a long-term fix. Because it doesn't rely on a core patch, it's more usable than #102 which has been my current recommendation.

I'm still hopeful that #136/#139 will be a good solution. I just need to put in the time to run it through its paces.

As someone stumbling on this thread from elsewhere, i'm super confused about why there are so many different patches, and which one i should use.

Can someone please update the IS to clarify what is going on and how to move forward here?

Agreed. I will work on the issue summary over the weekend. It will essentially be the information I gave above in #135 and #102.

But the short answer is:

The patch in #102 is small and works and but it hacks core. Thus, it's not a long-term solution.

The patch in #139 looks promising but needs some testing from the community. And hopefully a test case. I have been preoccupied over the past few months - I only do Drupal in my spare time and I haven't had a lot of that recently. If you can test this out and post some feedback here that would be highly appreciated.

The last submitted patch, 140: 2816033-use-database-connection-140.patch, failed testing. View results →

OK, that's helpful, thank you.

The patch (#80) showed up on a site I inherited, so honestly I don't even know why it's there.
Now I'm working on D10 updates for same, and this patch failed to apply, which led me to this thread.

I'll try 139 and see how it goes

Just ran into this problem, patch in 139 fixed it for me! Thank-you @budalokko and @TR!

Patch from #139 works for me, problem solved on my site.

I tried #139 on 3.0.0-alpha8 and got this error:
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "rules.event_subscriber.lazy". Did you mean this: "rules.event_subscriber"? in Drupal\Component\DependencyInjection\Container->get() (line 157 of core/lib/Drupal/Component/DependencyInjection/Container.php).

Check if web\modules\contrib\rules\src\EventSubscriber\LazyGenericEventSubscriber.php is here, where it should be. It might have ended up elsewhere.

I've created a test case that demonstrates the issue. Attached patch `2816033-no-subscribed-events-153-test-only.patch` only contains the test, so it should fail.

The other `2816033-no-subscribed-events-153-test-and-fix.patch` attachment contains the test and also mxh's fix from #136 and #139, so hopefully should pass.

Interdiff against #139.

Sry the noise.
Last test-only patch did not fail in CI. Here I try to visit `update.php` with the same path as core tests do.

The last submitted patch, 154: 2816033-no-subscribed-events-154-test-only.patch, failed testing. View results →

Previous issues were due to Drupal CI not matching current PHP minimal version :)

Attached again:

- A test only patch that demonstrates the issue.
- A test + the mxh lazy subscriber fix from #136-#139.
- interdiff against #139.

Only test results against PHP >= 8.1 should be taken into account please.

Removed the "Needs tests" patch as this should be complete now and ready for review. It's worth noting this is exactly the same patch in #136-#139 already working for 2 users. Just tests added.

The last submitted patch, 156: 2816033-no-subscribed-events-156-test-only.patch, failed testing. View results →

I had situation with where my rules were being applied inconsistently. I thought I might have issue with custom rules condition plugin, however I could not even step though and debug. After installing this patch, I am now able to debug, although it would seem issue is here not with custom module.

Drupal 10.2.2

Rules version: '8.x-3.0-alpha8'

Patch applies without issue.

Thanks

I moved the patch into an MR because that's easier for me to review on my work laptop. I think the changes are really close to being ready. I mainly wanted to raise the point about the code duplication, but then I found the comments that need to be updated.

I tried #156 and it solved my issue. Thanks!

I have been using this patch 156, and it seems to help.

Difficult to say definitively, but I have found that rules sometimes appear to be triggered on new node publish, other times they do not.

@chaitanyadessai could you explain why a second MR was necessary? I diffed the changes from the two MRs. The only real difference is that you incorrectly added spaces before some docblock delimiters.

Update: I removed the patch again. It turns out, that my comment #162 is a seperate issue.
So this is a 'go' for this patch from my side. Sorry for the wrong information. I will edit the comment and open a new issue.

Patch #156 has been successful here on a site where the rules stopped firing. Without the patch, disabling & re-enabling the rules would get them working for a short time, then they would silently stop firing again. With the patch, they have been firing OK for over a month.

I've also used #156 and that fixed the issue for me.

Getting the following error using the patch #156 :

TypeError: Argument 1 passed to Drupal\rules\Plugin\RulesAction\UserRoleAdd::doExecute() must implement interface Drupal\user\UserInterface, null given in Drupal\rules\Plugin\RulesAction\UserRoleAdd->doExecute() (line 52 of /var/www/html/docroot/modules/contrib/rules/src/Plugin/RulesAction/UserRoleAdd.php)

Drupal version : 9.5.11
Rules module version : 8.x-3.0-alpha8

Rules are getting invoked but unable to execute 'Add User Role' action.

After upgrading from Drupal 9.5.x to 10.2.5 I get the following error:

Error: Call to a member function toUrl() on string in node_tokens() (line 180 of core/modules/node/node.tokens.inc).

The issue is with my rule that sends an email after updating a node. The node context is being passed to the token replacement as a string "node" instead of a node object. When I back off this patch (#156) the token replacement is passed the node object as expected.

I'm experiencing the same issue. On my side, it is reproducable. After rebuilding the cache with Drush, the listener for my desired event is gone. \Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch does not return the event since it is not in the listener array. After editing the Rule and adding another random event, the desired event is being fired.

Patch #156 solves the issue.

Why does this issue still needs work? Looks more like a RTBC.

Rerolling patch for latest dev version of module.

drupal/core:^4.1
drupal/rules:dev-4.0.x#46815e3cca9fabd079f3b87e4dc5830e1aa28392

Sorry missed a bit in the rebase. rerolled working here.

mrconnerton → changed the visibility of the branch 8.x-3.x to hidden.

Sorry, patch still isn't perfect. Need to remove '@entity_type.manager' from rules.event_subscriber

Appreciating the attempt to update the MR. However, as long a the MR is not mergeable and does not properly show the relevant code parts that are requested to get merged, this cannot be reviewed. Thus setting to NW.

When providing and updated version of the patch, please also provide an interdiff file from the last one that got mostly perceived as working, which was the one from #156. In that comment you can also find an example of such an interdiff file. It's an important piece of the patching workflow in order to efficiently notice the differences for anybody. I know that the patch is most probably only provided for patching the codebase and not as part of the review process here, but still it should be included.

Adding a reroll_diff as requested.

I don't have the permissions to update the base target of the MR. Will wait for @dcam to do it else we can create a new MR.

@dcam can you please update the base target of this MR to 4.0.x

The target branch has been updated as requested.

MR is in mergeable state ready for review.

Thanks for updating the merge request.

Setting to NW because the tests are failing: https://git.drupalcode.org/issue/rules-2816033/-/jobs/3962405#L1017
The same error is happening often times in the tests. The test failure is actually happening since the merge request was created, so it's not because of the rebase that just happened.

It turns out the merge request that was initially created in #160 does not 100% reflect the patch of #156. It is different, seems the contents were manually copied over and some parts were forgotten or copy-pasted wrong.

What I can see for example is that this line https://git.drupalcode.org/project/rules/-/merge_requests/21/diffs?commi...
still includes the event_subscriber tag definition whereas within the patch of #156 this definition was removed. And this is most probably the reason why the tests are currently failing in the merge request, as the main point of the patch is to let the listeners being subscribed by LazyGenericEventSubscriber.

We first need to make sure it 100% reflects those changes from #156, then rebase (and then make further changes if needed).

The original MR was created February 1st 2024 against 8.x-3.x. I tried updating that MR / rebasing for 4.0.x but as indicated, the original MR commit wasn't created fully from the previous patch #156.

I don't want to keep messing with an MR I can't fully edit so I've created a new branch on the issue fork to reroll the #156 patch to the latest 4.0.x.

I've attached the patch and a reroll diff from 156-186.

@dcam had some comments on his original MR that I'm sure are up for discussion by others.

At the end of the day, I was just looking this patch to fix a problem #156 solves on our site rerolled for 4.0.x.

mrconnerton → changed the visibility of the branch 4.0.x to hidden.

mrconnerton → changed the visibility of the branch 2816033-listener-registration to hidden.

Just FYI: I updated the MR branch because the patch wasn't applying. It was only after the fact that I realized the patch doesn't apply to version 4.0.0. It did apply to the latest 4.0.x branch.