TypedConfigManagerInterface is not used everywhere - sometimes TypedConfigManager is which makes it impossible to swap out the service.

Plus the interface and the class documentation are not correct.

Files: 
CommentFileSizeAuthor
#15 2271967-psr4.patch20.1 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,632 pass(es). View
#13 2271967-psr4.patch20.1 KBJose Reyero
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,616 pass(es), 1 fail(s), and 0 exception(s). View
#10 2271967.10.patch20.64 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,615 pass(es). View
#10 8-10.patch4.43 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 8-10.patch. Unable to apply patch. See the log in the details link for more information. View
#8 2271967-8.patch25.44 KBamitgoyal
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2271967-8.patch. Unable to apply patch. See the log in the details link for more information. View
#6 2271967-6.patch1.89 KBamitgoyal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,744 pass(es). View
#4 2271967-4.patch25.17 KBJose Reyero
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,723 pass(es), 8 fail(s), and 14 exception(s). View
#2 2271967-2-interdiff.txt2.34 KBJose Reyero
#2 2271967-2.patch25.24 KBJose Reyero
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,724 pass(es). View
#1 2271967.1.patch24.58 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,767 pass(es). View

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
24.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,767 pass(es). View

The attached patch changes all typehints on TypedConfigManager to TypedConfigManagerInterface apart from COnfigImporter because there is another patch doing that.

If also changes the TypedConfigManager/TypedConfigManagerInterface to match the function signature of PluginManagerBase/TypedDataManager to make consildation at some point in the future a bit simpler. This has the added benefit of making the data returned from the TypedConfigManager since the parent is now set in the definitions.

Jose Reyero’s picture

FileSize
25.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,724 pass(es). View
2.34 KB

This looks pretty good to me.

Only one issue. The documentation of TypedConfingManagerInterface::createInstance() method doesn't match because of that $config array. But instead of fixing the documentation I'd rather update the code so it is still more consistent with TypedDataManager.

/**
   * Instantiates a typed configuration object.
   *
   * @param string $data_type
   *   The data type, for which a typed configuration object should be
   *   instantiated.
   * @param array $configuration
   *   The plugin configuration array, i.e. an array with the following keys:
   *   - data definition: The data definition array.
   *   - name: (optional) If a property or list item is to be created, the name
   *     of the property or the delta of the list item.
   *   - parent: (optional) If a property or list item is to be created, the
   *     parent typed data object implementing either the ListInterface or the
   *     ComplexDataInterface.
   *
   * @return \Drupal\Core\Config\Schema\Element
   *   The instantiated typed configuration object.
   */
  public function createInstance($data_type, array $configuration = array());

Also on the implementation itself there's one comment that is wrong, where it reads:

    // Allow per-data definition overrides of the used classes, i.e. take over
    // classes specified in the data definition.

it should say

    // Allow per-data definition overrides of the used classes, i.e. take over
    // classes specified in the type definition.

that is just the same as TypedDataManager.

I think the updated code also clears up the difference between 'data definition' and 'type definition'.

Attached updated patch and inter-diff.

There are other consistency issues, like that DataDefinition objects, and the 'list class' that were changed in TypedDataManager, but unfortunately not in TypedConfigManager, but I think that will better go in a follow up patch, since this one only changes internal parameters, but the other may have a bigger impact

alexpott’s picture

Status: Needs review » Needs work

@reyero - nice changes! One small thing...

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -106,7 +106,11 @@ public function create(array $definition, $value = NULL, $name = NULL, $parent =
     $definition['name'] = $name;
     $definition['parent'] = $parent;

This can be removed.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
25.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,723 pass(es), 8 fail(s), and 14 exception(s). View

@alexpott,

Right, updated patch with that change

This is the inter-diff.

--- a/core/lib/Drupal/Core/Config/TypedConfigManager.php
+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -104,8 +104,6 @@ public function create(array $definition, $value = NULL, $name = NULL, $parent 
       $definition['type'] = $this->replaceName($definition['type'], $replace);
     }
     // Create typed config object.
-    $definition['name'] = $name;
-    $definition['parent'] = $parent;
     $wrapper = $this->createInstance($definition['type'], array(
       'data_definition' => $definition,
       'name' => $name,

Status: Needs review » Needs work

The last submitted patch, 4: 2271967-4.patch, failed testing.

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
1.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,744 pass(es). View

Please review updated patch as the last patch #4 was not getting applied in current 8.x branch (2d3d33b48f901b1fedd9ad00f176e3bff17056ec). Below was the error,

$ git apply -v 2271967-4.patch
Checking patch core/lib/Drupal/Core/Config/Config.php...
Checking patch core/lib/Drupal/Core/Config/ConfigFactory.php...
Checking patch core/lib/Drupal/Core/Config/ConfigManager.php...
Hunk #1 succeeded at 36 (offset 1 lines).
error: while searching for:
   *   The entity manager.
   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   *   The configuration factory.
   * @param \Drupal\Core\Config\TypedConfigManager $typed_config_manager
   *   The typed config manager.
   * @param \Drupal\Core\StringTranslation\TranslationManager $string_translation
   *   The string translation service.
   */
  public function __construct(EntityManagerInterface $entity_manager, ConfigFactoryInterface $config_factory, TypedConfigManager $typed_config_manager, TranslationManager $string_translation, StorageInterface $active_storage) {
    $this->entityManager = $entity_manager;
    $this->configFactory = $config_factory;
    $this->typedConfigManager = $typed_config_manager;

error: patch failed: core/lib/Drupal/Core/Config/ConfigManager.php:60
error: core/lib/Drupal/Core/Config/ConfigManager.php: patch does not apply

I have made the given changes in core/lib/Drupal/Core/Config/ConfigManager.php and now it applies successfully.

Jose Reyero’s picture

Status: Needs review » Needs work

@amitgoyal,
Not sure I get it, the patch was much bigger than #6.

Anyway, I'll be re-rolling the patch in #4, for any core updates, and also fixing the issues found by the test

amitgoyal’s picture

FileSize
25.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2271967-8.patch. Unable to apply patch. See the log in the details link for more information. View

@Jose - Oh! I just provided the changes in file core/lib/Drupal/Core/Config/ConfigManager.php in my last patch at #6.

Attaching here the complete patch (#4 + #6).

Not very sure about the issues found by automated test script. So please guide me or fix them. Thanks!

Jose Reyero’s picture

@amitgoyal,
I think the only issue with the patch is that it expects 'parent' and 'name' to be part of the definition array. But they are not anymore (because of #3), nor they need to be there,

So this is just the test making a wrong assumption, just need to fix the test.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 8-10.patch. Unable to apply patch. See the log in the details link for more information. View
20.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,615 pass(es). View

reyero - are you sure they shouldn't be?

Here's a patch reverting the changes to core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTest.php

The last submitted patch, 10: 8-10.patch, failed testing.

Jose Reyero’s picture

@alexpott,
Yes, they shouldn't.

They used to be passed in the same parameters array , but not anymore, see
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21TypedData...

Note the only tests that fail are the ones that assume they are into the array, not the others, that actually test schema functionality.

Jose Reyero’s picture

FileSize
20.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,616 pass(es), 1 fail(s), and 0 exception(s). View

Re-rolled the patch in #10 for PSR-4 update.

Status: Needs review » Needs work

The last submitted patch, 13: 2271967-psr4.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
20.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,632 pass(es). View

re-uploading patch to preserve test failure for #2276213: Random fail in ApcuBackendUnitTest due to fast test bot.

Jose Reyero’s picture

I think this is ready.

We need this committed to move on with #1928868: Typed config incorrectly implements Typed Data interfaces

Gábor Hojtsy’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community
Issue tags: +D8MI, +language-config, +sprint

I agree this is a much needed cleanup. It looks like API changes but its documenting the API better with the expected methods documented on the interface instead of just being there :D

The last submitted patch, 8: 2271967-8.patch, failed testing.

Gábor Hojtsy’s picture

15: 2271967-psr4.patch queued for re-testing.

  • Commit 973170d on 8.x by catch:
    Issue #2271967 by Jose Reyero, alexpott, amitgoyal: Fixed and use...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

vijaycs85’s picture

Issue tags: -sprint

nice improvement and the interface looks much better now.

Status: Fixed » Closed (fixed)

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