Problem/Motivation

Configuration files must be either lists or maps, i.e. a configuration file that contains just a scalar value is invalid. In particular, the configuration schema of config objects is either a Sequence or a Mapping. SchemaCheckTrait validates this by checking whether the typed config objects are instances of ArrayElement - the common parent class of the two implementations. This is a fairly arbitrary check, though.

Proposed resolution

What we really are validating is that the schema definition object is traversable. Therefore we should have an interface for typed data objects that are traversable: TraversableTypedDataInterface.

ComplexDataInterface and ListInterface should extend TraversableTypedDataInterface and ArrayElement should implement it.

Remaining tasks

User interface changes

API changes

Comments

tstoeckler’s picture

Title: ArrayElement should implement ComplexDataInterface » Introduce a TraversableTypedDataInterface and use that for typehinting instead of ArrayElement
Issue summary: View changes
Status: Active » Needs review
FileSize
10.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,586 pass(es). View

So in fact, I was wrong. ComplexDataInterface maps to Mapping, but ListInterface (which does not extend ComplexDataInterface) maps to Sequence. So this patch instead introduces a TraversableTypedDataInterface which is just TraversableTypedDataInterface extends TypedDataInterface, \Traversable which can be used for type-hinting, and which both ComplexDataInterface and ListInterface extend.

Let's see how badly this breaks.

tstoeckler’s picture

Issue tags: +Amsterdam2014
Jose Reyero’s picture

I agree that we shouldn't be using ArrayElement for that, but not sure we need to invent a new interface.

Could we just check whether it implements \Traversable ? Or it implements \Traversable && \TypedDataInterface ?

Jose Reyero’s picture

Forget my previous question
@tstoeckler exlplained it to me in person to me and the reason is function getArrayTranslation(TraversableTypedDataInterface $element...)

So I'm all for this one, it is a simple patch and makes a lot of sense...

... and also we could be using it in some more places in typed data like

/**
 * @file
 * Contains \Drupal\Core\TypedData\TypedDataInterface.
 */

...

interface TypedDataInterface {

  /**
   * Returns the parent data structure; i.e. either complex data or a list.
   *
   * @return \Drupal\Core\TypedData\ComplexDataInterface|\Drupal\Core\TypedData\ListInterface
   *   The parent data structure, either complex data or a list; or NULL if this
   *   is the root of the typed data tree.
   */
  public function getParent();

...
}

All the typed data's parent parameters could/should (?) be this type, I think.

tstoeckler’s picture

FileSize
1.15 KB
12.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,728 pass(es). View

I just realized that I totally forgot to follow-up on this, sorry @Jose Reyero.

Nice catch on the TypedDataInterface::getParent(). Updated that and ::getRoot() as well.

Btw, a note on some of the seemingly unrelated changes: I went through all usages of ArrayElement to find whether they were still legit and therefore included a few easy clean-ups even though their not necessary strictly speaking.

Jose Reyero’s picture

It looks great to me, just two minor things.

-  protected function getElementTranslation($element, array $options) {
+  protected function getElementTranslation(TypedDataInterface $element, array $options) {
     $translation = array();
-    if ($element instanceof ArrayElement) {
+    if ($element instanceof \Traversable) {
       $translation = $this->getArrayTranslation($element, $options);
     }

Not sure whether this is on purpose (using \Traversable), it looks ok anyway.

Should we change this one too, not really important, but maybe yes just for consistency since getParent() returns TraversableTypedDataInterface...?

interface TypedDataInterface {

  /**
   * Constructs a TypedData object given its definition and context.
...
   * @param \Drupal\Core\TypedData\TypedDataInterface $parent
   *   (optional) The parent object of the data property, or NULL if it is the
   *   root of a typed data tree. Defaults to NULL.,
   *
   */
  public static function createInstance($definition, $name = NULL, TypedDataInterface $parent = NULL);

Whatever I think the patch is ready to go, so we get rid of 'instanceof ArrayElement', whether we use it for all other things in TypedDataInterface, is not that important IMO..

Jose Reyero’s picture

vijaycs85’s picture

Issue tags: +D8MI, +sprint
tstoeckler’s picture

FileSize
2.41 KB
13.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Here we go.

CanIHazRTBC? :-P

Status: Needs review » Needs work

The last submitted patch, 9: 2346129-9-array-element-typed-data.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
570 bytes
14.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Maybe this time?

Status: Needs review » Needs work

The last submitted patch, 11: 2346129-11-array-element-typed-data.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
797 bytes
14.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,373 pass(es), 1 fail(s), and 0 exception(s). View

...

Status: Needs review » Needs work

The last submitted patch, 13: 2346129-13-array-element-typed-data.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +language-config
vijaycs85’s picture

may just need this:

index 5556c6e..0628c89 100644
--- a/core/tests/Drupal/Tests/Core/Entity/TypedData/EntityAdapterUnitTest.php
+++ b/core/tests/Drupal/Tests/Core/Entity/TypedData/EntityAdapterUnitTest.php
@@ -252,7 +252,7 @@ public function testGetParent() {
    */
   public function testSetContext() {
     $name = $this->randomMachineName();
-    $parent = $this->getMock('\Drupal\Core\TypedData\TypedDataInterface');
+    $parent = $this->getMock('\Drupal\Core\TypedData\TraversableTypedDataInterface');
     // Our mocked entity->setContext() returns NULL, so assert that.
     $this->assertNull($this->entityAdapter->setContext($name, $parent));
     $this->assertEquals($name, $this->entityAdapter->getName());
tstoeckler’s picture

Status: Needs work » Needs review
FileSize
858 bytes
15.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,369 pass(es). View

Maybe this one?

andypost’s picture

Green! +1 to RTBC

vijaycs85’s picture

+++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
@@ -6,17 +6,12 @@
-use \ArrayAccess;
-use \ArrayIterator;
-use \Countable;
-use \IteratorAggregate;
-use \Traversable;
...
+abstract class ArrayElement extends Element implements \IteratorAggregate, TraversableTypedDataInterface, \ArrayAccess, \Countable {

@@ -111,7 +106,7 @@ public function count() {
+    return new \ArrayIterator($this->getElements());

Looks like irrelevant to this issue. In fact the old format (i.e. use) looks like the right way.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

@vijaycs85 you are correct that this is theoretically not necessary but I was changing the implements line anyway, so...
And our coding standards dictate the leading backslash for classes in the global name space.

RTBC per #18

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 221a48c and pushed to 8.0.x. Thanks!

diff --git a/core/lib/Drupal/Core/Config/Schema/ArrayElement.php b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
index 4a3b896..8becdd7 100644
--- a/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
+++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
@@ -44,7 +44,7 @@ protected function getAllKeys() {
   /**
    * Builds an array of contained elements.
    *
-   * @return \Drupal\Core\TypedData\TypedDataInterface
+   * @return \Drupal\Core\TypedData\TypedDataInterface[]
    *   An array of elements contained in this element.
    */
   protected abstract function parse();

Minor code doc fix on commit.

  • alexpott committed 221a48c on 8.0.x
    Issue #2346129 by tstoeckler: Introduce a TraversableTypedDataInterface...
tstoeckler’s picture

Yay, thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.