Place block operation in the modal Place block dialog does not bring up config dialog

This stuff is happening deep in the rendering system. Not clear where the Translation object is getting into the mix.

@claudiu.cristea: I'm not sure how to come up with a simpler example. I've tried to create a simple render array that passes different things to a #markup item. t() doesn't cause the problem, and using theme object that loads twig hasn't done it (although it's a twig object that does it).

On the system that displays this, the problem survives a cache rebuild.

I"m going to try a little more with the debugger, so I can see who introduces the TranslatableMarkup into the render process.

After some checking in the debugger, here's what I know:

1. Blocks displays condition configuration tabs of any Condition plugins it can find.
2. The strings in question come in via the plugin annotations.
3. The problem strings are from @Translation() annotations for plugin context
4. While I do have Rules installed, the "note_type" condition plugin from core also passes in a non-string. On my system, I'm having problems with 'node_type' from Core, and 'rules_data_comparison' from Rules.

Annotation from Note Type plugin (\Drupal\node\Plugin\Condition\NodeType):

/**
 * Provides a 'Node Type' condition.
 *
 * @Condition(
 *   id = "node_type",
 *   label = @Translation("Node Bundle"),
 *   context = {
 *     "node" = @ContextDefinition("entity:node", label = @Translation("Node"))
 *   }
 * )
 *
 */

Annotation from Data Comparison plugin (\Drupal\rules\Plugin\Condition\DataComparison):

/**
 * Provides a 'Data comparison' condition.
 *
 * @Condition(
 *   id = "rules_data_comparison",
 *   label = @Translation("Data comparison"),
 *   category = @Translation("Data"),
 *   context = {
 *     "data" = @ContextDefinition("any",
 *       label = @Translation("Data to compare"),
 *       description = @Translation("The data to be checked to be empty, specified by using a data selector, e.g. 'node:uid:entity:name:value'.")
 *     ),
 *     "operator" = @ContextDefinition("string",
 *       label = @Translation("Operator"),
 *       description = @Translation("The comparison operator."),
 *       required = FALSE
 *     ),
 *     "value" = @ContextDefinition("any",
 *        label = @Translation("Data value"),
 *        description = @Translation("The value to compare the data with.")
 *     )
 *   }
 * )
 *
 * @todo: Add access callback information from Drupal 7.
 * @todo: Find a way to port rules_condition_data_is_operator_options() from Drupal 7.
 */

The bad conversion seems to occur at a different place than I thought earlier.

The problem occurs in
core/themes/classy/templates/form/form-element.html.twig

while Twig tries to escape the following string:

description = @Translation("The data to be checked to be empty, specified by using a data selector, e.g. 'node:uid:entity:name:value'."))

The exception is thrown in core/lib/Drupal/Core/Template/TwigExtension.php at line 441;

        throw new \Exception(t('Object of type "@class" cannot be printed.', array('@class' => get_class($arg))));

The routine wants to convert any object it's handed as an object via either ::__toString() or ::toString(). The translation object has neither, and the method throws.

I'm not sure why TranslatableMarkup has no toString() method. For all I know, it might be by design.

This is starting to look like a Twig rendering bug. There's nothing wrong with how the Rules Condition plugin is declared AFAICT, but it would appear that either TranslatableMarkup needs string conversion method, or that Twig needs to have code to explicitly handle that case.

Still odder: class TranslatableMarkup has a ::__toString() method via trait ToStringTrait. In a stand-alone drush script, this is discovered, but it is not discovered when this is tested in TwigExtension.php.

Not sure why this could be, but it's possible that we need the extra toString() method as well.

OK, I'm not sure if I have the right fix, but I have a fix.

This is incorrect:

it seems that validateUtf8() wrongly receives a TranslatableMarkup object instead of a string.

Close, but in reality, the routine was passed a Translation annotation, and not TranslatableMarkup.

This is an example of a translatable object that is not TranslatableMarkup. It wraps a TranslatableMarkup, but is not one itself.

I enclose a patch that deals with the Annotation case. I'm not sure if I should be handling a Translation annotation or AnnotationInterface. For now, I'm doing the latter.

Not sure what to do here for a test.

@claudiu.cristea:

No, sorry. This patch is hiding the cause of the bug. An Annotation object should be not passed ever in $arg. You need to check the caller.

That would be the Block Module. Sorry ;-)

The calling code is in BlockForm::buildVisibilityInterface(), which calls into Core's condition system as so:

      $condition = $this->manager->createInstance($condition_id, isset($visibility[$condition_id]) ? $visibility[$condition_id] : []);
      $form_state->set(['conditions', $condition_id], $condition);
      $condition_form = $condition->buildConfigurationForm([], $form_state);

by this point, the Translation annotation objects are already in the render stream. You will find that no contrib code is called anywhere in that chain. I've been trying to figure out how exactly the process works, but so far I haven't found it.

You can argue whether this is a block bug. But it's certainly a core bug. And the UI problem will appear as a Block UI problem.

@claudiu.cristea -- not exactly. I did find a Rules bug, but core was suppressing Translation objects, which is part of why you didn't see the crash on ajax.

The Rules issue had to do with matching criteria between contexts. I'll log it a bit later.

Core was effectively ignoring both label and description strings (which are always Translate annotations). The offending code looks like this:

Drupal\Core\Annotation\ContextDefinition::__construct(), from line 112:

    // Annotation classes extract data from passed annotation classes directly
    // used in the classes they pass to.
    foreach (['label', 'description'] as $key) {
      // @todo Remove this workaround in https://www.drupal.org/node/2362727.
      if (isset($values[$key]) && $values[$key] instanceof TranslatableMarkup) {
        $values[$key] = (string) $values[$key]->get();
      }
      else {
        $values[$key] = NULL;
      }
    }

In practice, label and description are never actually TranslatableMarkup, so this code was never used. They are always set to NULL right now.

I'd recommend doing two things here:

1. Implement a __toString() method for Translate.
2. Change the code in ContextDefinition to also handle annotation objects.

I've already done a pass on this.

Also, @chx had asked me to see if I could come up with some test code. That I also have. The bug in Rules exposed the count($options) > 1 block in ContextAwarePluginAssignmentTrait. This was hidden because Core does not have enough context providers to ever get $options with more than 1 item.

The test code is a small module that implements a simple context provider, and a plugin that actually uses label and description. This should be simple enough to make the problem understandable.

Some simple test code per @chx: https://github.com/torenware/test_contexts.

This does two things:

1. It creates another context provider, which will cause the Role condition to display the $options UI (it does not do so now).
2. It implements a NodeIsMine condition plugin that should display a description and a label if ContextDefinition is changed to not eat those two Translate annotations.

Just turn the sucker on, and look at how the different context tabs in Blocks render differently.

Added a patch to allow description and label Translation annotations to actually be used in the UI. YMMV :-)

@klausi -- there's a subtle difference between the Core class and the Rules substitute.

The Rules class was allowing Translate objects out, which is why we saw the crash with Rules installed, but not with Core alone.

The Core class attempts to prevent the crash by looking for the label and description keys, but the work-around is not correct: it assumes the items are TranslatableMarkup, which they are not. Core should accept the patch in #18 to get correct behavior. The current behavior is wrong because it strips away strings that are actually needed to display the UI right.

If Core wants to pass on the patch, we can do the same thing in our Rules substitute class. If we correctly turn the Translation objects into strings, then at least we will not see a problem when Rules is installed. I can create a patch for Rules to do this if you like.