Drupal\Core\Template\Attribute->createAttributeValue does not always return instance of \Drupal\Core\Template\AttributeValueBase

Ok, upon running a local test I discovered that a TranslationWrapper was passed in as the value argument. Let's look at the function since it's fairly short...

  protected function createAttributeValue($name, $value) {
    // If the value is already an AttributeValueBase object, return it
    // straight away.
    if ($value instanceOf AttributeValueBase) {
      return $value;
    }
    // An array value or 'class' attribute name are forced to always be an
    // AttributeArray value for consistency.
    if (is_array($value) || $name == 'class') {
      // Cast the value to an array if the value was passed in as a string.
      // @todo Decide to fix all the broken instances of class as a string
      // in core or cast them.
      $value = new AttributeArray($name, (array) $value);
    }
    elseif (is_bool($value)) {
      $value = new AttributeBoolean($name, $value);
    }
    elseif (!is_object($value)) {
      $value = new AttributeString($name, $value);
    }
    assert('$value instanceof \\Drupal\\Core\\Template\\AttributeValueBase', 'invalidReturn');
    return $value;
  }

If we pass an object to this function then, without the assertion, it will be returned without conversion. Is this the correct behavior.

If yes, the assertion must be changed to allow this.
If no, the code must be changed to forbid it and all calling code debugged.

Or, is the intent that an object's string expression be returned as an AttributeString? If so another branch to the conditional is called for.

That last one is the one I'll try for patching this.

Added a conditional to force an object that has a magic __toString method to cast itself as a string. This satisfies both the two failed tests and the assertion added for the Template Assertions patch. This patch will be added to it's parent so that the parent can pass testing.

It would help if I'd apply against core.

Probably should add a test to the code that is directly affected - then again in a sense the assertion is itself a test of sorts. I'll look into adding a test tomorrow.

Well, here's the problem...

  /**
   * Tests set of values.
   */
  public function testSet() {
    $attribute = new Attribute();
    $attribute['class'] = array('example-class');

    $this->assertTrue(isset($attribute['class']));
    $this->assertEquals(new AttributeArray('class', array('example-class')), $attribute['class']);
  }

That test only checks if arrays get converted. It checks no other types and doesn't check invalids. I'll go ahead an add that logic, but I need to get the assert statement in at this point to check to make sure it gets thrown when invalid objects are supplied. The only thing that can do that is outside code, which indicates a bug in that code, so assert is the correct handle here, not an exception.

K, multiple new tests added to make sure what I understand the code should be returned is being returned. Also a test to see if the assertion gets thrown.

No interdiff since the first patch was only 3 lines.

I'm not sure. The point of the assertion is to enforce the PHPDocs comment - that the function only returns objects of a certain type. It has proven that in at least 4 cases in the existing code that isn't happening. I don't know what is more disruptive - enforcing the PHP docs statement or dropping the assert - but I do know the long term health of the code is best served by enforcing the PHP docs or changing them to match the actual behavior.

Something else that isn't tested for - Numbers of all types. Reading the code flow they will currently drop through and be returned unmodified. Why where they overlooked? Are they even legal inputs? Are they legal only for certain attributes - I know you can't have a class name that is merely a number, nor an ID, and they can't start with a number either. Is it the responsibility of this class to make that check?

Assuming they are legal, do we create a new AttributeValue container for them, or cast them to string and let the AttributeString class work with them?

This morning I started doing a lot more hardening shuffling, so this is no longer a simple issue but does go hand in hand with assertions. Closing this ticket to merge back to the Template Assertions ticket.