Problem/Motivation

While Typed Configuration builds on Typed Data, there are still a few mismatches between both of them, and this should add some consistency in how interfaces and parameters are used in both sides.

There's currently a TypedConfigManagerInterface, but the counterpart in TypedData doesn't exist. Because of this, some methods are defined twice, once in TypedConfigManagerInterface, once in TypedDataManager.

This helps us to address some @todo in \Drupal\Core\TypedData\TypedData: "Add the typed data manager as proper dependency."

Proposed resolution

1. Add a new interface: TypedDataManagerInterface
2. Use it for typed parameters instead of the class TypedDataManager
3. Get TypedConfigManagerInterface to extend TypedDataManagerInterface
4. Then use the new interface in TypedDataTrait and use it for fixing the dependency issues in TypedData objects
5. More cleanup is possible in #2533776: Instead of mocking TypedDataManager, use TypedDataManagerInterface in tests once this lands.

Remaining tasks

Commit.

User interface changes

None.

API changes

Minimal. Use TypedDataManagerInterface instead of TypedDataManager.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because the issue implement OO best practices by using an interface instead of demanding a specific class where TypedDataManager is needed.
Issue priority Normal because while the change makes understanding the typed data system much easier, its not a showstopper.
Disruption Minimal. Use TypedDataManagerInterface instead of TypedDataManager to typehint. There is little use in core, and only pretty specialized code depends on it.
Files: 
CommentFileSizeAuthor
#51 33-51-interdiff.txt964 bytesalexpott
#51 2488568-51.patch48.69 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2488568-51.patch. Unable to apply patch. See the log in the details link for more information. View
#47 2488568.47.patch45.19 KBJose Reyero
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,623 pass(es). View
#47 2488568.44-47.diff.txt1000 bytesJose Reyero
#44 2488568-36-44-diff.txt1.35 KBJose Reyero
#44 2488568.44.patch44.21 KBJose Reyero
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#36 2488568.36.patch44.68 KBJose Reyero
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#36 2488568-36-33-diff.txt4.09 KBJose Reyero
#33 2488568.33.patch48.52 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,931 pass(es). View
#33 29-33-interdiff.txt1.36 KBalexpott
#29 2488568.29.patch48.29 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,918 pass(es). View
#29 28-29-interdiff.txt3.98 KBalexpott
#28 2488568-28.patch42.69 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,839 pass(es). View
#22 2488568-typed_data_manager_interface-22-19-diff.txt1.59 KBJose Reyero
#22 2488568-typed_data_manager_interface-22.patch42.73 KBJose Reyero
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2488568-typed_data_manager_interface-22.patch. Unable to apply patch. See the log in the details link for more information. View
#16 interdiff.txt667 bytesGábor Hojtsy
#16 2488568-16.patch42.54 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,227 pass(es). View
#15 2488568-15.patch42.44 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,137 pass(es), 1 fail(s), and 0 exception(s). View
#7 2488568-typed_data_manager_interface-02-03-diff.txt7.08 KBJose Reyero
#7 2488568-typed_data_manager_interface-03.patch42.37 KBJose Reyero
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,227 pass(es). View
#3 2488568-typed_data_manager_interface-02.patch32.22 KBJose Reyero
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2488568-typed_data_manager_interface-02.patch. Unable to apply patch. See the log in the details link for more information. View
#1 2488568-typed_data_manager_interface-01.patch38.34 KBJose Reyero
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,772 pass(es). View

Comments

Jose Reyero’s picture

Status: Active » Needs review
FileSize
38.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,772 pass(es). View

Full patch for review and testing.

Gábor Hojtsy’s picture

Issue tags: +API change
Jose Reyero’s picture

Title: Add a TypedDataManagerInterface / Use it for proper dependencies in TypedData » Add a TypedDataManagerInterface / Use it for typed parameters
Issue summary: View changes
FileSize
32.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2488568-typed_data_manager_interface-02.patch. Unable to apply patch. See the log in the details link for more information. View

Trimming down this patch, so it is more straight forward and way easier to review:
- Just add TypedDataManagerInterface and use it for typed parameters.
- Leave the dependency issues for a follow up.

Updating title and issue description too.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config, +sprint

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

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

++ 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: 2488568-typed_data_manager_interface-02.patch, failed testing.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
42.37 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,227 pass(es). View
7.08 KB

Re-rolled the patch for latest core updates and done all of the suggestions from @tstoeckler #5
- Also used @inheritdoc for TypedDataManager::getPropertyInstance()

@tstoeckler,
Not sure about the @inheritdoc + additional parameters documentation, is this what you mean?

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs beta evaluation

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.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs beta evaluation

Added beta evaluation. :)

jibran’s picture

Component: base system » typed data system
Issue tags: +Needs subsystem maintainer review

Let's ask subsystem maintainer for a review.

Berdir’s picture

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.

Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
42.44 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,137 pass(es), 1 fail(s), and 0 exception(s). View

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

Gábor Hojtsy’s picture

FileSize
42.54 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,227 pass(es). View
667 bytes

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

Gábor Hojtsy’s picture

Title: Add a TypedDataManagerInterface / Use it for typed parameters » Add a TypedDataManagerInterface and use it for typed parameters
Issue summary: View changes

The last submitted patch, 15: 2488568-15.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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().

alexpott’s picture

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.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
42.73 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2488568-typed_data_manager_interface-22.patch. Unable to apply patch. See the log in the details link for more information. View
1.59 KB

Re-rolled latest @Gábor Hojtsy patch #16, fixing:
- Issues pointed out by @alexpott on #22

About @alexpott #20 (clarify relationship between TypeDataManager and TypedConfigManager):

- Replaced getTypedDataManager() by getTypedConfigManager() in ArrayElement::createElement() for consistency, though that doesn't really address the question..

- 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().

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the fixes, they look good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2488568-typed_data_manager_interface-22.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2488568-typed_data_manager_interface-22.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
42.69 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,839 pass(es). View

Rerolled.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.98 KB
48.29 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,918 pass(es). View

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.

Gábor Hojtsy’s picture

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.

tstoeckler’s picture

Assigned: Unassigned » rfay

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).

tstoeckler’s picture

Assigned: rfay » Unassigned

Oops, the assignment was not intentional.

alexpott’s picture

FileSize
1.36 KB
48.52 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,931 pass(es). View

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.
tstoeckler’s picture

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.

Jose Reyero’s picture

@tstoeckler,

we have to either add setTypedDataManager() to TypedDataInterface

Yes, that's a different bug, but we could fix it here too

About the 'assert' in #33... why not throwing an Exception? I thought assertions were not something to use at runtime..? Otherwise, all of @alexpott's changes make a lot of sense.

About that interfaces and getTypedDataManager()/getTypedConfigManager() I would be more interested in fixing the methods/interface consistency than the documentation itself. Could we do something like:

function getTypedConfigManager() {
  if ($this->typedDataManager instanceof TypedConfigManagerInterface) {
    return $this->typedDataManager;
 }
 else {
   return \Drupal::service('config.typed');
 }
}

Anyway, I am looking into some other solutions like moving TypedConfigManager::buildDataDefinition() -which is the only method preventing us from using TypedDataManagerInteface everywhere - into the Element class, creating some TypedConfigDataDefinition and implementing it there, or maybe creating some new service to build that data definition objects from configuration schema + data, which may make sense since building these dynamic definitions is a whole different thing. All of the options take a lot of refactoring though, so I'm nowhere closer to a real patch for any of them...

Jose Reyero’s picture

FileSize
4.09 KB
44.68 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

So... forgetting about more complex options, and aiming at interface consistency, this is a re-roll from #33 which:

- Adds TypedConfigInterface::getTypedConfigManager() as explained in #35 This is just an extra method in the interface and provides exactly what we are looking for, a properly typed return value.

- Adds add setTypedDataManager() to TypedDataInterface as suggested by @tstoeckler #34

Other doc changes in TypedDataManagerInterface, not in the interdiff, are due to this one that just got in the middle #2548265: Improve TypedDataManager doxygen

tstoeckler’s picture

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 ...)

Gábor Hojtsy’s picture

@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 :(

Jose Reyero’s picture

I think the issue is explained in #22, "The reason why we need getTypedConfigManager()..."

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

tstoeckler’s picture

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.

alexpott’s picture

Issue tags: +rc target
Gábor Hojtsy’s picture

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

alexpott’s picture

Issue tags: -rc target +rc deadline

Fix the tag

Jose Reyero’s picture

FileSize
44.21 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
1.35 KB

Ok, let's remove the method from the interface and let that logic for the ArrayElement class internals.

Status: Needs review » Needs work

The last submitted patch, 44: 2488568.44.patch, failed testing.

Jose Reyero queued 36: 2488568.36.patch for re-testing.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
1000 bytes
45.19 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,623 pass(es). View

That failing test is pretty ugly, I don't really get it, anyway, trying to fix it....

PHP Fatal error: Class Drupal\Tests\serialization\Unit\Normalizer\TestComplexData contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\Core\TypedData\TypedDataInterface::setTypedDataManager) in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/serialization/tests/src/Unit/Normalizer/ComplexDataNormalizerTest.php on line 146

The last submitted patch, 36: 2488568.36.patch, failed testing.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
  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.

alexpott’s picture

FileSize
48.69 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2488568-51.patch. Unable to apply patch. See the log in the details link for more information. View
964 bytes

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.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yay, agreement!

effulgentsia’s picture

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

effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 8.0.x.

  • effulgentsia committed b6707fc on 8.0.x
    Issue #2488568 by Jose Reyero, alexpott, Gábor Hojtsy, tstoeckler: Add a...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks all!

Status: Fixed » Needs work

The last submitted patch, 51: 2488568-51.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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

Xano’s picture