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

Ran into issues with node_has_term condition from the Islandora mod… #819

Conversation

nigelgbanks
Copy link
Member

…ule. It

always saves to the block and fails to evaluate to false. It should not save if
no value has been specified for term URI.

Found the following in web/core/lib/Drupal/Core/Condition/ConditionPluginCollection.php.

// In order to determine if a plugin is configured, we must compare it to
// its default configuration. The default configuration of a plugin does
// not contain context_mapping and it is not used when the plugin is not
// configured, so remove the context_mapping from the instance config to
// compare the remaining values.
unset($instance_config['context_mapping']);
if ($default_config === $instance_config) {
  unset($configuration[$instance_id]);
}

When doing the comparison we have.

$default_config = [
  "id" => "node_has_term",
  "logic" => "and",
  "negate" => false
];
$instance_config = [
  "id" => "node_has_term",
  "logic" => "and",
  "negate" => false,
  "uri" => NULL,
];

So the comparison fails and the configuration is stored and evaluated later
causing issues since it fails to pass evaluation.

Hence we need to change the default config to match the submitted values.

What does this Pull Request do?

Changes the defaults for NodeHasTerm to reflect the submitted values.

What's new?

Example:

  • Configuration will not persist if matches defaults

How should this be tested?

A description of what steps someone could take to:

  • Submit a form where the configuration is present without changing values in the form
  • View the entity with devel
  • Note that configuration is persisted although was not set by the user

After change is applied the configuration is not persisted.

Additional Notes:

This will not remove persisted configuration that already exists without a value as === is used in core for the comparison of the configuration, and that takes the order of values into account, and that configuration will not match the defaults as uri was appended to the structure afterwards.

@nigelgbanks
Copy link
Member Author

Needed this as it was prevented some of my entities from being visible though I had not configured specified values for the condition.

…ule. It

always saves to the block and fails to evaluate to false. It should not save if
no value has been specified for term URI.

Found the following in `web/core/lib/Drupal/Core/Condition/ConditionPluginCollection.php`.

```php
// In order to determine if a plugin is configured, we must compare it to
// its default configuration. The default configuration of a plugin does
// not contain context_mapping and it is not used when the plugin is not
// configured, so remove the context_mapping from the instance config to
// compare the remaining values.
unset($instance_config['context_mapping']);
if ($default_config === $instance_config) {
  unset($configuration[$instance_id]);
}
```

When doing the comparison we have.

```php
$default_config = [
  "id" => "node_has_term",
  "logic" => "and",
  "negate" => false
];
$instance_config = [
  "id" => "node_has_term",
  "logic" => "and",
  "negate" => false,
  "uri" => NULL,
];
```

So the comparison fails and the configuration is stored and evaluated later
causing issues since it fails to pass evaluation.

Hence we need to change the default config to match the submitted values.
@nigelgbanks nigelgbanks force-pushed the do-not-persist-node-has-term-if-not-set branch from 9b0a956 to 5a0dbf7 Compare January 6, 2021 15:07
Copy link

@ajstanley ajstanley left a comment

Choose a reason for hiding this comment

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

Sensible defaults - what's not to love?

@dannylamb dannylamb merged commit ae2aecc into Islandora:8.x-1.x Jan 13, 2021
@nigelgbanks nigelgbanks deleted the do-not-persist-node-has-term-if-not-set branch April 2, 2021 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants