Clarify the logic in TypedDataManager::getPropertyInstance()

Looks great

This issue is a normal task that intends to reduce fragility, and per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?, we should discuss whether the benefit outweighs disruption. I would like to see a +1 from @fago or @berdir here as entity system maintainers (of which I assume typed data to be a part).

Personally I'm in favour of this issue because the discussions in #2221577: Fix assumption that field settings is not a nested array did reveal that this code is confusing.

Setting back to needs review to have said discussion.

@yched yep - see #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? for the allowed changes. There's a really good flowchart that explains it.

I have to defer to @fago on this, he knows this part better and currently only have very limited core time, so I can't go into depth here.

But I see nothing with changing this in general.

+1 to unify that kind of hashing

In general, +1 on the change. It makes the code clearer and easier to follow, but has no impact to the outside. Well unless people run into crc32 collisions now, but I think that's unlikely enough. ;-)

Here a review:

1. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
   @@ -248,26 +247,31 @@ public function getInstance(array $options) {
   +      $parts[] = crc32(serialize($settings));
   

   I cannot tell on which hashing algo to use, but looks like this has been discussed already. However, as it was something worth a discussion I think it also deserves a comment explaining our thinking.

2. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
   @@ -277,17 +281,16 @@ public function getPropertyInstance(TypedDataInterface $object, $property_name,
   +      // Create the prototype with initial parenting information, but without
   +      // any value.
          $this->prototypes[$key] = $this->create($definition, NULL, $property_name, $object);
   

   The parenting information differs by clone and will be overwritting later by setContext() - thus it does not make sense to set it here. To avoid confusion it shoul be removed and rely on parenting info being set later.

Discussed 2. with yched on IRC. I wrongly assumed that the patch introduces the pass $object parameter, but it doesn't. Looking closer at it, we shoud keep it such that constructors have enough parenting information to setup objects. But as this is not obvious, let's have that documented here.

Looks great to me.

New comments are great, thanks. Agreed this is ready.

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

I think this internal refactoring is justified given the complexity of the code and moving to the consistent hashing for non cryptographic purposes makes sense too.

This patch makes this easier to maintain, consistent with other non cryptographic hashing and less fragile. Committed f503eb9 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.