Method FieldItemList::equals() uses type safe comparison limiting usefulness

For storing values, we definitely want to rather save too much than lose data. It's a best effort implementation.

I agree!

But FieldItemList::equals() became part of the field API and can therefor used elsewhere and not just to decide if we run an update statement on the storage. In this case it should do what it's documentation states - in a reliable way.
Personally I really appreciate this new API functionality and therefor it would be bad if we turn it into a somehow "private" function.

Let me think about it ...

@hchonov: I thought about a kind of normalization of field values, too. But this will cause a bigger change.

What about a simple change that allows to "select" the desired behavior:

interface FieldItemListInterface {
  ...

  /**
   * Determines equality to another object implementing FieldItemListInterface.
   *
   * @param \Drupal\Core\Field\FieldItemListInterface $list_to_compare
   *   The field item list to compare to.
   *
   * @param bool $strict
   *   TRUE means that the values will be compared strictly.
   *
   * @return bool
   *   TRUE if the field item lists are equal, FALSE if not.
   */
  public function equals(FieldItemListInterface $list_to_compare, $strict = FALSE);

  ...
}

Where does the type casting occur? Does it occur on the way back out of the database?

@alexpott: Have a look at Drupal\system\Tests\Entity\ContentEntityChangedTest::testChanged() in the patch posted at https://www.drupal.org/node/2428795#comment-9852123.
This test failed and the examples in this issue's description are var_dump() during this test.

The timestamp is form the "created" field, the boolean from "language_default" and the same happens for "user_id" with an integer type:

array(1) {
  [0] => 
  array(1) {
    'target_id' =>
    int(1)
  }
}

array(1) {
  [0] => 
  array(1) {
    'target_id' =>
    string '1' (length=1)
  }
}

Did you get these type differences when just loading and manipulating the entity or is a form submission involved?

In this test, no form handling is included, just the entity API and the entity SQL storage.

shouldn't == be good enough because if we're relying anywhere on the variable type this shouldn't be down to the storage

The arguments in #4 seems to be in the same direction. As well as https://www.drupal.org/node/2297817#comment-9852673
Let's see what the testbot says to such a simple change ...

@joelpittet: you're right with that example. The string 'all' gets converted to integer in this context. That's PHP.

But I think that in our case we're just comparing "compatible" types like 0 == '0' or 0 == FALSE that could be converted into each other. And the array keys matter as well.

The patch in #9 passed. How good is the test coverage? ;-)

@alexpott asked me to check where these type conversions are coming from. Basically they're caused by PDOStatement which returns every value as a string. That's an example of a result for a simple article:

object(stdClass)[965]
  public 'vid' => string '1' (length=1)
  public 'langcode' => string 'en' (length=2)
  public 'revision_timestamp' => string '1430315259' (length=10)
  public 'revision_uid' => string '1' (length=1)
  public 'revision_log' => string '' (length=0)
  public 'nid' => string '1' (length=1)
  public 'type' => string 'article' (length=7)
  public 'uuid' => string 'b41b9844-993a-4401-9100-a79e979f1fb2' (length=36)
  public 'isDefaultRevision' => string '1' (length=1)

These values are directly passed to entities' __construct() and cached internally in $this->values as strings without any type conversion.

From my point of view a type conversion within PHP makes no sense. Even if you assign a "PHP type" to each field the amount of places where you have to ask the field definition for the type and you have to convert the value accordingly seems to be endless.

Therefor the simple patch from #9 seems to be the right solution.

Here's a test that covers the SQL storage load issue as described in #15. This one should fail.

And now the patches from #9 and #17 combined.

I extended the FieldItemListTest unit test In addition to the FieldSqlStorageTest from #17.
So we now have two different test that should fail without the patch from #9.

And now again the patch from #9 in combination with both tests.

I removed the kernel test as requested by @alexpott in IRC, because that will be covered by related issues.