Issue #2799361 - Port condition expression action links to the UI. #475
Conversation
| $rules_ui_handler = $this->getRulesUiHandler(); | ||
| $row['operations'] = [ | ||
| 'data' => [ | ||
| '#type' => 'dropbutton', |
There was a problem hiding this comment.
Technically not part of the new changes, but there's a dedicated Operations render element that extends from Dropbutton.
There was a problem hiding this comment.
The dropdown was part of drag/drop PR #452 that this PR is using. But knowing that the operations element adds a theme suggestion is a good reason to change it.
| */ | ||
| private function mirrorElements($values) { | ||
| $elements = NULL; | ||
| $expressionManager = \Drupal::service('plugin.manager.rules_expression'); |
There was a problem hiding this comment.
Ideally this service would be introduced via dependency injection, rather than doing the global call. As an example, see ContentEntityForm.
There was a problem hiding this comment.
I agree that dependency injection would be ideal. The reason its not implemented with DI is because the form is being called in a non-standard way (not from the container). Its getting a single argument passed into the constructor, and so we would have to go further up into the api to affect that. I wanted to stay away from making api changes as much as possible.
There was a problem hiding this comment.
Ah, okay. Maybe abstract it out into a separate method, e.g.
$expressionManager = $this->getExpressionManger();
...
function getExpressionManger() {
return \Drupal::service('plugin.manager.rules_expression');
}
Mainly I'm just trying to keep options open for unit testing.
| @@ -27,7 +27,17 @@ protected function allowsMetadataAssertions() { | |||
| * {@inheritdoc} | |||
| */ | |||
| public function executeWithState(ExecutionStateInterface $state) { | |||
| // Get hold of actions. | |||
| // @todo See if we can add getExpressions method of ExpressionContainerBase. | |||
| $actions = []; | |||
There was a problem hiding this comment.
Is it really necessary to put in an extra loop just for the copying to a local variable? Can't you just
$actions = $this->actions; or even just sort the class property directly? (Ditto for the other applicable classes.)
There was a problem hiding this comment.
Actually I think its necessary because $this->actions is really an iterable object and not an array. This is part of the code coming from the included PR #452.
There was a problem hiding this comment.
Hmm...looks like an array to me:
/**
* List of actions that will be executed.
*
* @var \Drupal\rules\Engine\ActionExpressionInterface[]
*/
protected $actions = [];
...
public function __construct(array $configuration, $plugin_id, $plugin_definition, ExpressionManagerInterface $expression_manager) {
...
foreach ($configuration['actions'] as $action_config) {
$action = $expression_manager->createInstance($action_config['id'], $action_config);
$this->actions[] = $action;
}
}
But at any rate, if it's part of the other PR, then I won't split hairs here.
| '@plugin' => $this->conditionContainer->getLabel(), | ||
| '@config-plugin' => $config->bundle(), | ||
| '%label' => $config->label(), | ||
| '@url' => 'http://drupal.org/node/1300034', |
There was a problem hiding this comment.
There is a newer placeholder type that should be used here, I think: https://www.drupal.org/node/2571689
There was a problem hiding this comment.
Good to know about the newer placeholder. I assume we are talking about the @url element? Since its not user provided input I wouldn't normally worry about XSS escaping for a hard coded url, but I can change it to use the new placeholder quick enough that its a moot point.
dsasser
force-pushed
the
feature/2799361
branch
2 times, most recently
from
b12c4f9 to
26f2dcf
Compare
- Replaced dropbutton with operation elements - Used new :placeholder for url placeholder values - Move expressionManager to class property
dsasser
force-pushed
the
feature/2799361
branch
from
26f2dcf to
75b4cc9
Compare
… from component changes during save.
Adds the ability to group conditions within AND and OR condition sets in the UI. I used the patch from #452 to provide drag and drop functionality to the form and modified from there. See the d.o issue at: https://www.drupal.org/node/2799361