Skip to content
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

Change reaction-rule config to save the event with settings #405

Open
wants to merge 11 commits into
base: 8.x-3.x
Choose a base branch
from
36 changes: 30 additions & 6 deletions config/schema/rules.schema.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,21 @@ rules.reaction.*:
label:
type: label
label: 'Label'
event:
type: string
label: 'Event'
events:
type: sequence
label: 'Events'
sequence:
type: mapping
label: 'Event'
mapping:
event_name:
type: string
label: 'Name'
configuration:
type: sequence
label: 'Configuration'
sequence:
type: mapping
module:
type: string
label: 'Module'
Expand Down Expand Up @@ -194,9 +206,21 @@ rules_expression.rules_reaction_rule:
uuid:
type: string
label: 'UUID'
event:
type: string
label: 'Event'
events:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated, this seems wrong / outdated. Probably we can remove rules_expression.rules_reaction_rule - the plugin does not exist any more afair.

type: sequence
label: 'Events'
sequence:
type: mapping
label: 'Event'
mapping:
event_name:
type: string
label: 'Name'
configuration:
type: sequence
label: 'Configuration'
sequence:
type: mapping
conditions:
type: rules_expression.[id]
label: 'Conditions'
Expand Down
11 changes: 6 additions & 5 deletions src/Engine/RulesComponent.php
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,12 @@ public function getContextDefinitions() {
*/
public function addContextDefinitionsFrom(ConfigEntityInterface $rules_config) {
if ($rules_config instanceof ReactionRuleConfig) {
$event_name = $rules_config->getEvent();
// @todo Use setter injection for the service.
$event_definition = \Drupal::service('plugin.manager.rules_event')->getDefinition($event_name);
foreach ($event_definition['context'] as $context_name => $context_definition) {
$this->addContextDefinition($context_name, $context_definition);
foreach ($rules_config->getEvents() as $event) {
// @todo Use setter injection for the service.
$event_definition = \Drupal::service('plugin.manager.rules_event')->getDefinition($event['event_name']);
foreach ($event_definition['context'] as $context_name => $context_definition) {
$this->addContextDefinition($context_name, $context_definition);
}
}
}
return $this;
Expand Down
18 changes: 11 additions & 7 deletions src/Entity/ReactionRuleConfig.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
* config_export = {
* "id",
* "label",
* "event",
* "events",
* "module",
* "description",
* "tag",
Expand Down Expand Up @@ -121,11 +121,15 @@ class ReactionRuleConfig extends ConfigEntityBase {
protected $module = 'rules';

/**
* The event name this reaction rule is reacting on.
* The events this reaction rule is reacting on.
*
* @var string
* Events array, key - numeric index, value - event array with next structure:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be a full sentence alway:
The array is numerically indexed and contains arrays with the following structure:

* - event_name: string with the event machine name.
* - configuration: an array containing the event configuration.
*
* @var array
*/
protected $event;
protected $events;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should initialize this as empty array with "[]".


/**
* Sets a Rules expression instance for this Reaction rule.
Expand Down Expand Up @@ -208,10 +212,10 @@ public function getTag() {
}

/**
* Returns the event on which this rule will trigger.
* Returns the array of events on which this rule will trigger.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misses detailed @return documentation. Also, let's better describe the method as "Gets configuration of all events the rule is reacting on." - so it's more clear, that the structure is some configuration structure.

Also, for a lot of code only the event names are from interest (e.g. ReactionRuleStorage), so I think there should be a getter getEventNames() which returns the array of fully qualified event names of the rule.

*/
public function getEvent() {
return $this->event;
public function getEvents() {
return $this->events;
}

/**
Expand Down
18 changes: 11 additions & 7 deletions src/Entity/ReactionRuleStorage.php
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,10 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
protected function getRegisteredEvents() {
$events = [];
foreach ($this->loadMultiple() as $rules_config) {
$event = $rules_config->getEvent();
if ($event && !isset($events[$event])) {
$events[$event] = $event;
foreach ($rules_config->getEvents() as $event) {
if ($event && !isset($events[$event['event_name']])) {
$events[$event['event_name']] = $event['event_name'];
}
}
}
return $events;
Expand All @@ -106,10 +107,13 @@ public function save(EntityInterface $entity) {

// After the reaction rule is saved, we need to rebuild the container,
// otherwise the reaction rule will not fire. However, we can do an
// optimization: if the event was already registered before, we do not have
// to rebuild the container.
if (empty($events_before[$entity->getEvent()])) {
$this->drupalKernel->rebuildContainer();
// optimization: if every event was already registered before, we do not
// have to rebuild the container.
foreach ($entity->getEvents() as $event) {
if (empty($events_before[$event['event_name']])) {
$this->drupalKernel->rebuildContainer();
break;
}
}

return $return;
Expand Down
2 changes: 1 addition & 1 deletion src/EventSubscriber/GenericEventSubscriber.php
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public function onRulesEvent(Event $event, $event_name) {
// another rule.
foreach ($triggered_events as $triggered_event) {
// @todo Only load active reaction rules here.
$configs = $storage->loadByProperties(['event' => $triggered_event]);
$configs = $storage->loadByProperties(['events.*.event_name' => $triggered_event]);

// Loop over all rules and execute them.
foreach ($configs as $config) {
Expand Down
3 changes: 2 additions & 1 deletion src/Form/ReactionRuleAddForm.php
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ public function form(array $form, FormStateInterface $form_state) {
}
}

$form['event'] = [
$form['events']['#tree'] = TRUE;
$form['events'][]['event_name'] = [
'#type' => 'select',
'#title' => $this->t('React on event'),
'#options' => $options,
Expand Down
17 changes: 11 additions & 6 deletions src/Form/ReactionRuleEditForm.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,17 @@ public static function create(ContainerInterface $container) {
public function form(array $form, FormStateInterface $form_state) {
$this->addLockInformation($form);

$event_name = $this->entity->getEvent();
$event_definition = $this->eventManager->getDefinition($event_name);
$form['event']['#markup'] = $this->t('Event: @label (@name)', [
'@label' => $event_definition['label'],
'@name' => $event_name,
]);
foreach ($this->entity->getEvents() as $key => $event) {
$event_definition = $this->eventManager->getDefinition($event['event_name']);
$form['event'][$key] = [
'#type' => 'item',
'#title' => $this->t('Events') . ':',
'#markup' => $this->t('@label (@name)', [
'@label' => $event_definition['label'],
'@name' => $event['event_name'],
]),
];
}
$form_handler = $this->entity->getExpression()->getFormHandler();
$form = $form_handler->form($form, $form_state);
return parent::form($form, $form_state);
Expand Down
44 changes: 39 additions & 5 deletions tests/src/Kernel/EventIntegrationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public function testUserLoginEvent() {
$config_entity = $this->storage->create([
'id' => 'test_rule',
'expression_id' => 'rules_rule',
'event' => 'rules_user_login',
'events' => [['event_name' => 'rules_user_login']],
'configuration' => $rule->getConfiguration(),
]);
$config_entity->save();
Expand All @@ -81,7 +81,7 @@ public function testUserLogoutEvent() {
$config_entity = $this->storage->create([
'id' => 'test_rule',
'expression_id' => 'rules_rule',
'event' => 'rules_user_logout',
'events' => [['event_name' => 'rules_user_logout']],
'configuration' => $rule->getConfiguration(),
]);
$config_entity->save();
Expand All @@ -108,7 +108,7 @@ public function testCronEvent() {
$config_entity = $this->storage->create([
'id' => 'test_rule',
'expression_id' => 'rules_rule',
'event' => 'rules_system_cron',
'events' => [['event_name' => 'rules_system_cron']],
'configuration' => $rule->getConfiguration(),
]);
$config_entity->save();
Expand All @@ -134,7 +134,7 @@ public function testSystemLoggerEvent() {
$config_entity = $this->storage->create([
'id' => 'test_rule',
'expression_id' => 'rules_rule',
'event' => 'rules_system_logger_event',
'events' => [['event_name' => 'rules_system_logger_event']],
'configuration' => $rule->getConfiguration(),
]);
$config_entity->save();
Expand All @@ -161,7 +161,7 @@ public function testInitEvent() {
$config_entity = $this->storage->create([
'id' => 'test_rule',
'expression_id' => 'rules_rule',
'event' => KernelEvents::REQUEST,
'events' => [['event_name' => KernelEvents::REQUEST]],
'configuration' => $rule->getConfiguration(),
]);
$config_entity->save();
Expand All @@ -185,4 +185,38 @@ public function testInitEvent() {
$this->assertRulesLogEntryExists('action called');
}

/**
* Test that rules config supports multiple events.
*/
public function testMultipleEvents() {
$rule = $this->expressionManager->createRule();
$rule->addCondition('rules_test_true');
$rule->addAction('rules_test_log');

$config_entity = $this->storage->create([
'id' => 'test_rule',
'expression_id' => 'rules_rule',
'events' => [
['event_name' => 'rules_user_login'],
['event_name' => 'rules_user_logout'],
],
'configuration' => $rule->getConfiguration(),
]);
$config_entity->save();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should test using the getter also.


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

$account = User::create(['name' => 'test_user']);
// Invoke the hook manually which should trigger the rules_user_login event.
rules_user_login($account);
// Invoke the hook manually which should trigger the rules_user_logout
// event.
rules_user_logout($account);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice that this works already!


// Test that the action in the rule logged something.
$this->assertRulesLogEntryExists('action called');
$this->assertRulesLogEntryExists('action called', 1);
}

}