Problem/Motivation

ContextDefinitionInterface::getLabel and ContextDefinitionInterface::getDescription claims to return strings but after enabling the Rules module they return the @Translation annotation object which causes a fatal in twig. Notably the MockBlockManager does return new ContextDefinition($data_type, (string) $label, $required); so there is evidence of non-string labels causing problems.

Proposed resolution

No idea.

Remaining tasks

Identify how the annotation got there.

User interface changes

API changes

Data model changes

Comments

Torenware created an issue. See original summary.

Torenware’s picture

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

claudiu.cristea’s picture

@Torenware, it seems that validateUtf8() wrongly receives a TranslatableMarkup object instead of a string. But I cannot reproduce the error. Can you give more details on how to reproduce the bug?

Torenware’s picture

@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.

claudiu.cristea’s picture

Status: Active » Postponed (maintainer needs more info)

Ah, so it's not pure Drupal core but your module. Because I tested a lot to catch the error. So, it seems that is nothing wrong with Drupal core but maybe an error of implementation on your side?

Postponing this while we have more info :)

Torenware’s picture

Status: Postponed (maintainer needs more info) » Active

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.
 */
Torenware’s picture

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.

Torenware’s picture

Issue tags: +translatable strings
Torenware’s picture

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.

Torenware’s picture

Status: Active » Needs review
Issue tags: +Annotation, +Twig
FileSize
997 bytes

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’s picture

Status: Needs review » Postponed (maintainer needs more info)

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. Why the caller is passing that object? That is a bug. But I don't think is the core doing this but other modules that you've installed or your custom module.

Torenware’s picture

@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’s picture

OK. Please provide the exact steps to reproduce the error. But I suggest you to test this on fresh Drupal 8.0.x-dev installation with no module installed, other than those from the current profile.

The steps from the issue summary didn't reveal any error for me.

chx’s picture

Issue summary: View changes
Status: Postponed (maintainer needs more info) » Active
claudiu.cristea’s picture

So, this seems more like a Rules block sending wrong arguments to block system?

Torenware’s picture

@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.

Torenware’s picture

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.

Torenware’s picture

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

klausi’s picture

Rules uses its own ContextDefinition annotation class. Not sure what is going on here, maybe we did not swap out the core ContextDefinition correctly?

Can we reproduce this somehow in core alone and add that to the issue summary?

Torenware’s picture

@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.

claudiu.cristea’s picture

Project: Drupal core » Rules
Version: 8.0.x-dev » 8.x-3.x-dev
Component: block.module » Rules Core
Status: Active » Needs review
Issue tags: -Ajax, -unicode, -internationalization, -translatable strings, -Twig
FileSize
1.19 KB

As I said this is a Rules issue.

EDIT: I've implemented the workaround copied from \Drupal\Core\Annotation\ContextDefinition::__construct() and that should be removed when #2362727: Implement __toString() on Translation Annotation lands.

claudiu.cristea’s picture

FileSize
1.52 KB

Forgot the use statement.

claudiu.cristea’s picture

Priority: Normal » Critical

And I guess this is critical while it breaks the whole Block placement functionality.

Status: Needs review » Needs work

The last submitted patch, 22: 2587179-22.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
922 bytes

OK. Fixed the failure on tests except \Drupal\Tests\rules\Kernel\ContextIntegrationTest. That one makes the HEAD broken and does not belong to this patch. See the branch test broken https://www.drupal.org/pift-ci-job/56053.

Status: Needs review » Needs work

The last submitted patch, 25: 2587179-25.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review

The only failure is the branch test failure. This patch is ready for review.

chx’s picture

Priority: Critical » Major

Yes this is a bad bug but it's no way critical: there's no data loss or security hole and the error is not even reproducible with core AFAICS.

chx’s picture

Priority: Major » Critical

Oh sorry I didn't realize this was moved into Rules. Nevermind me :)

klausi’s picture

Status: Needs review » Needs work

Please file a pull request instead, see the Rules dev workflow: https://thefubhy.gitbooks.io/rules/content/contributing/start_developing...

claudiu.cristea’s picture

That is a 404 link.

klausi’s picture

Works for me, did you click on a sliced off link in your email inbox?

claudiu.cristea’s picture

claudiu.cristea’s picture

Status: Needs work » Needs review
klausi’s picture

Status: Needs review » Needs work

Cool, we should have some kind of test case for this. #2610458: Prevent Translation annotation objects from getting into the render system has a comment that shows a possibility where we assert that we are actually dealing with strings. So I think this is a duplicate of #2610458: Prevent Translation annotation objects from getting into the render system, correct?

klausi’s picture

Status: Needs work » Closed (duplicate)

I think this is now a duplicate of #2610458: Prevent Translation annotation objects from getting into the render system, the block placing UI works for me.