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
38 changes: 15 additions & 23 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 @@ -184,26 +196,6 @@ rules_expression.rules_rule:
type: rules_expression.[id]
label: 'Actions'

rules_expression.rules_reaction_rule:
type: rules_expression
label: "Reaction Rule"
mapping:
id:
type: string
label: 'Plugin ID'
uuid:
type: string
label: 'UUID'
event:
type: string
label: 'Event'
conditions:
type: rules_expression.[id]
label: 'Conditions'
actions:
type: rules_expression.[id]
label: 'Actions'

rules.context.definition:
type: mapping
label: 'Context definition'
Expand Down
41 changes: 33 additions & 8 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,16 @@ 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. 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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a code smell if we need to document array keys. I think we should use a simple class instead which has this properties + methods documented. We should prevent nested array weirdness when we can.

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed, but can we do that with CMI? I've only seen it save/load arrays.
As alternative, we could use an order array with

$event_name => $configuration

which Alex tried initially. Problem there was that we need to have dots in event names (as symfony events use dots), but CMI does not allow dots in config keys. Thus, we'd have to massage the event names and somehow escape/replace dots. That makes it more complicated, also when querying for events etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we can file a follow-up to explore using objects instead of event arrays.

*
* @var array
*/
protected $event;
protected $events = [];

/**
* Sets a Rules expression instance for this Reaction rule.
Expand Down Expand Up @@ -169,7 +174,7 @@ public function getExpression() {
*/
public function getComponent() {
$component = RulesComponent::create($this->getExpression());
$component->addContextDefinitionsForEvents([$this->getEvent()]);
$component->addContextDefinitionsForEvents($this->getEventNames());
return $component;
}

Expand Down Expand Up @@ -237,10 +242,30 @@ public function getTag() {
}

/**
* Returns the event on which this rule will trigger.
* Gets configuration of all events the rule is reacting on.
*
* @return array
* The events array. 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.
*/
public function getEvents() {
return $this->events;
}

/**
* Gets fully qualified names of all events the rule is reacting on.
*
* @return string[]
* The array of fully qualified event names of the rule.
*/
public function getEvent() {
return $this->event;
public function getEventNames() {
$names = [];
foreach ($this->events as $event) {
$names[] = $event['event_name'];
}
return $names;
}

/**
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->getEventNames() as $event_name) {
if (!isset($events[$event_name])) {
$events[$event_name] = $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->getEventNames() as $event_name) {
if (empty($events_before[$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 @@ -73,12 +73,17 @@ protected function prepareEntity() {
public function form(array $form, FormStateInterface $form_state) {
$form['locked'] = $this->rulesUiHandler->addLockInformation();

$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->getEventNames() as $key => $event_name) {
$event_definition = $this->eventManager->getDefinition($event_name);
$form['event'][$key] = [
'#type' => 'item',
'#title' => $this->t('Events:'),
'#markup' => $this->t('@label (@name)', [
'@label' => $event_definition['label'],
'@name' => $event_name,
]),
];
}
$form_handler = $this->rulesUiHandler->getComponent()
->getExpression()->getFormHandler();
$form = $form_handler->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',
]);
$config_entity->set('events', [
['event_name' => 'rules_user_login'],
['event_name' => 'rules_user_logout'],
]);
$config_entity->set('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);
}

}