Add a TypedDataManagerInterface and use it for typed parameters

Bring this on the D8MI sprint, even though not strictly language related.

++ for this issue!

Marking RTBC as this issue is certainly an improvement over the status quo.

There are a couple things that could be improved and IMO fall into this scope, so I wouldn't mind them being fixed here, but we can also improve in a follow-up.

1. TypedConfigManager::get() could have its documentation removed in favor of @inheritdoc
2. In TypedDataManagerInterface::createInstance() we should aknowledge the inheritance from FactoryInterface by replacing the one-line doc with a @inheritdoc. I think it makes sense to leave the method in for documentation and type-hinting purposes, but it's not really a method that TypedDataManagerInterface is introducing so we should be clear about that.
3. The docs on TypedDataManager::getInstance() should be replaced with @inheritdoc. If we want to keep the specific documentation we should put it on TypedDataManagerInterface (like in 2.)
4. I think the ordering of the methods in TypedDataManagerInterface could use some love. I know that you just used the same ordering as in TypedDataManager but e.g. having setters before getters feels fairly unnatural to me.

Yes, that's what I meant. Thanks! Looks even better now.

This looks really sensible to me - going to ping @fago and @berdir for a once over before proceeding. Also as this is a normal task we need a beta evaluation.

Added beta evaluation. :)

Let's ask subsystem maintainer for a review.

I think this looks fine. The issue summary doesn't seem 100% up to date, it looks like the issue is already doing more than described there and already gets rid of the dependency and doesn't just add the interface.

Looks like we have around 6 calls to "$this->getMockBuilder('\Drupal\Core\TypedData\TypedDataManager')" that could be slightly simplified now that we have an interface. A follow-up to do that would be nice.

Rerolled because this did not apply likely due to #2527708: Improve documentation for TypedConfigManagerInterface.

Removed the obsolete @todo as identified by @berdir. Also removed a superfluous newline among the traits used.

Opened #2533776: Instead of mocking TypedDataManager, use TypedDataManagerInterface in tests for @berdir's concern.

Fail in #15 was unrelated (in menu test, "The test did not complete due to a fatal error."). All feedback addressed, and I only changed two lines in very minor ways, so back to RTBC.

Thanks everyone - this is a nice place to get to. Any improvement that clarifies the relationship between TypeDataManager and TypedConfigManager reduces fragility.

+++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
@@ -133,6 +126,13 @@ public function onChange($name) {
+  public function getTypedConfigManager() {
+    return $this->getTypedDataManager();

@@ -153,7 +153,7 @@ public function getIterator() {
+    return $this->getTypedDataManager()->create($definition, $value, $key, $this);

@@ -171,20 +171,8 @@ protected function createElement($definition, $value, $key) {
+    return $this->getTypedConfigManager()->buildDataDefinition($definition, $value, $key, $this);

Having both the methods seems confusing. There is only one usage of getTypedConfigManager().

diff --git a/core/lib/Drupal/Core/Config/Schema/ArrayElement.php b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
index 28e4b56..43274b3 100644
--- a/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
+++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
@@ -8,7 +8,6 @@
 namespace Drupal\Core\Config\Schema;
 
 use Drupal\Component\Utility\SafeMarkup;
-use Drupal\Core\Config\TypedConfigManagerInterface;
 use Drupal\Core\TypedData\TypedData;
 
 /**
diff --git a/core/lib/Drupal/Core/Config/Schema/TypedConfigInterface.php b/core/lib/Drupal/Core/Config/Schema/TypedConfigInterface.php
index c6e4cf4..9fdad61 100644
--- a/core/lib/Drupal/Core/Config/Schema/TypedConfigInterface.php
+++ b/core/lib/Drupal/Core/Config/Schema/TypedConfigInterface.php
@@ -64,7 +64,7 @@ public function toArray();
   /**
    * Gets the typed configuration manager.
    *
-   * @return \Drupal\Core\TypedData\TypedConfigManagerInterface
+   * @return \Drupal\Core\Config\TypedConfigManagerInterface
    *   The typed data manager.
    */
   public function getTypedConfigManager();
diff --git a/core/lib/Drupal/Core/Config/TypedConfigManager.php b/core/lib/Drupal/Core/Config/TypedConfigManager.php
index 1490b47..f9e5e60 100644
--- a/core/lib/Drupal/Core/Config/TypedConfigManager.php
+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -10,7 +10,6 @@
 use Drupal\Component\Utility\NestedArray;
 use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Core\Cache\CacheBackendInterface;
-use Drupal\Core\Config\Schema\ArrayElement;
 use Drupal\Core\Config\Schema\ConfigSchemaAlterException;
 use Drupal\Core\Config\Schema\ConfigSchemaDiscovery;
 use Drupal\Core\Extension\ModuleHandlerInterface;

Unused uses and a reference to a class that does not exist.

Thanks for the fixes, they look good.

Views's DisplayTest fails there with Drupal\views\Plugin\views\style\StylePluginBase: Missing row plugin, does not seem to be related.

Rerolled.

In reviewing this patch I've realise we have more problems. We can't fall back to TypedDataTrait::getTypedDataManager() because that gets the wrong manager. Also I really do not see the harm in ArrayElement overriding the TypedDataTrait::getTypedDataManager(). In fact, I think that all config typed data should extend from Element and Element should override the set and get of the typed data manager to ensure we're working with the correct one. As in the patch attached. We can enforce a narrowing of the interface on the setter using an assert.

I'm sorry that this is going to take a bit longer.

I don't know if there was/is a reason for Element not to be a base class for ArrayElement, I'm fine with the suggestions.

Yay, I think that makes the patch a lot more understandable.

I have two minor nitpicks:

1. +++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
   @@ -7,27 +7,10 @@
   +abstract class ArrayElement extends Element implements \IteratorAggregate, TypedConfigInterface {
   

   I checked and all other config elements (whose existence is similarly questionable to Element before this patch, but I digress...) already extend from Element, so we should be fine. ++

2. +++ b/core/lib/Drupal/Core/Config/Schema/Element.php
   @@ -21,4 +22,38 @@
   +   * @param \Drupal\Core\TypedData\TypedDataManagerInterface $typed_data_manager
   

   This should be \Drupal\Core\Config\TypedConfigManagerInterface

3. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
   @@ -108,39 +93,13 @@ public function createInstance($data_type, array $configuration = array()) {
   +    $typed_data = $class::createInstance($data_definition, $configuration['name'], $configuration['parent']);
   +    $typed_data->setTypedDataManager($this);
   +    return $typed_data;
   

   Yay!!!!!!

4. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManagerInterface.php
   @@ -2,81 +2,23 @@
   -class TypedDataManager extends DefaultPluginManager {
   ...
   +Interface TypedDataManagerInterface extends PluginManagerInterface, CachedDiscoveryInterface {
   

   *I*nterface should be lowercase

   (but neat trick with the copy-detection)

Otherwise looks good to go from my perspective, but I think @Jose Reyero should sign off on the interdiff (thus leaving at needs review).

Oops, the assignment was not intentional.

Thanks @tstoeckler

1. Yep
2. I think this is for Element::setTypedDataManager() - this one is a bit tricky because we can't type hint on the TypedConfigManagerInterface cause PHP wants this to be compatible with TypedDataTrait::setTypedDataManager. This is why the assertion exists. I guess we can improve the documentation on the @param - but an @param that does not match the typehint in the method is too ood.
3. :)
4. Fixed.

Hmmm... I don't think a mismatch in type-hint and documentation is very confusing. When working with the entity system I do that a lot, in fact, because I often don't have what I need in e.g. EntityInterface but that's forced by the interface. Anyway, I guess we can assume that people can read and the documentation is crystal clear, so that works for me.

I noticed one thing earlier, though:

+    $typed_data = $class::createInstance($data_definition, $configuration['name'], $configuration['parent']);
+    $typed_data->setTypedDataManager($this);
+    return $typed_data;

We now call a setTypedDataManager() method on an object we know is an instance of TypedDataInterface but (in theory) don't know much more. In other words, if people provide typed data plugins without subclassing TypedData they will get fatals. So I think we have to either add setTypedDataManager() to TypedDataInterface or add a TypedDataManagerAwareInterface which we can check against.

Still leaving at "needs review" for input from @Jose Reyero.

Hmmm... not really sure what to do here. I personally find #35/ #36 less clear than @alexpott's patch because the subtle difference between "I am returning a typed data manager" and "I am returning a typed *config* manager" is... well, really subtle. I am also not clear on what concretely the problem is with only having a single method for "both" (where one is only a specialized version of the other, so "both" isn't really fitting here, but ...)

@tstoeckler can you follow up on this one? looks like this is blocked on reviews and your concerns specifically with the time window rapidly closing on it if not already :(

Re #39:

So #22 says:

- The reason why we need getTypedConfigManager() and getTypedDataManager() is because we need to use the buildDataDefinition() method that is not part of the TypedDataManagerInterface and if we do something like:

    return $this->getTypedDataManager()->buildDataDefinition($definition, $value, $key, $this);

That would work, because the TypedDataManager used for config objects is an instance of TypedConfigManager (see TypedDataManager::createInstance()) but it would be wrong because we would be calling a method that is not part of the interface that is returned by getTypedDataManager().

I think the patch in #33 explains this situation sufficiently:

+  /**
+   * Gets the typed configuration manager.
+   *
+   * Overrides \Drupal\Core\TypedData\TypedDataTrait::getTypedDataManager() to
+   * ensure the typed configuration manager is returned.
+   *
+   * @return \Drupal\Core\Config\TypedConfigManagerInterface
+   *   The typed configuration manager.
+   */
+  public function getTypedDataManager() {

I realize that having to document something like this makes it evident that this is not an ideal situation, but I think the patch in #36 also comes with a cost in terms of understandability because with it you can retrieve two different objects which implement the same interface (by extension) and because of the inheritance of the actual classes that implement the interfaces it actually *is* the same thing that happens, but they're still two different objects retrieved with two different objects.

Let's figure this out ASAP and get in before RC.

Fix the tag

99% of this is just introducing an interface, typehinting to that interface, and moving docblocks from the implementation to the interface.

I'm no TypedData expert, but this seems like an innocent enough change that it's okay for me to RTBC it.

I only found two nits, that can easily be fixed on commit:

1. +++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
   @@ -138,6 +121,22 @@ public function getIterator() {
    
   +
   

   Nit: one extraneous \n here.

2. +++ b/core/lib/Drupal/Core/Config/Schema/Element.php
   @@ -8,6 +8,7 @@
   +use Drupal\Core\TypedData\TypedDataManagerInterface;
   

   Nit: Unnecessary change.

1. +++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
   @@ -7,27 +7,10 @@
   -use Drupal\Core\Config\TypedConfigManagerInterface;
   
   @@ -138,6 +121,22 @@ public function getIterator() {
   +    if ($this->typedDataManager instanceof TypedConfigManagerInterface) {
   

   Using it, but removing it.

2. +++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
   @@ -138,6 +121,22 @@ public function getIterator() {
   +    if ($this->typedDataManager instanceof TypedConfigManagerInterface) {
   

   There is no use so the instanceof always fails.

3. +++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
   @@ -152,7 +151,7 @@ public function getIterator() {
   -    return $this->typedConfig->create($definition, $value, $key, $this);
   

   This looks really really odd - using getTypedDataManager() in one place and getTypedConfigManager() in another. Looks very dangerous.

Personally I think we should just go back to #33 and remove the assert and through an exception.

Rerolled #33 and changed the assert to not use the single quote eval style.

I think @tstoeckler wrote in #34 and #40 that the patch in #33 is clearer and I agree. Also in #39 @Jose Reyero wrote:

Anyway, ok, we can live without that kind of consistency, I think there are many of them in D8 atm..

I think given that both @tstoeckler and I feel that #33 is clearer and @Jose Reyero can live with it we should go with it.

Yay, agreement!

Looks good to me. Ticking the credit box for @tstoeckler.

Pushed to 8.0.x.

Yay, thanks all!

This change may have caused #2615790: Field item properties do not prevent the services they contain from being serialized.