Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
766 bytes

Initial patch...

vijaycs85’s picture

Few documentation improvements as per @tstoeckler.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +blocker

Awesome, thanks @vijaycs85!

Also tagging this as blocker. We currently have this as a procedural method in config_translation but A) this is in no way specific to config_translation and B) procedural function --

So let's get this out of the way.

Gábor Hojtsy’s picture

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

You know this needs tests. A simple one checking a config schema that does not exist and one that does would be great. I think we may have in fact some test for a similar thing that can be extended.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
2.15 KB
2.35 KB

Yeah, I guess tests can't hurt. And it was especially easy with your help! Thanks again.

tstoeckler’s picture

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -298,7 +298,7 @@ public function hasConfigSchema($name) {
-    return is_array($definition) && $definition['class'] != '\Drupal\Core\Config\Schema\Property';
+    return is_array($definition) && ($definition['class'] != '\Drupal\Core\Config\Schema\Property');

Oh yeah, that is not really necessary, but I hate to stare at something for 5 seconds until I grok it, and I think it's clearer this way.

Status: Needs review » Needs work

The last submitted patch, 2097259-has-config-schema-5.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.74 KB
2.35 KB

That's embarassing. As soon as there's no autocompletion, I get 3 fatals for 3 LoC.

tstoeckler’s picture

Damn, I really need some sleep. The interdiff is correct, but this should have been the patch. #8 is identical to #5.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +language-config
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +API addition

Lets create a TypedConfigManagerInterface as we're adding a public method to an object.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
3.43 KB

Thanks for the review @alexpott. Here is the interface with Manager update.

vijaycs85’s picture

Minor doc comment update...

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yay! Resolves the above concerns :)

Status: Reviewed & tested by the community » Needs work
Issue tags: -API addition, -D8MI, -language-config, -blocker

The last submitted patch, 2097259-has-config-schema-13.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +API addition, +D8MI, +language-config, +blocker

#13: 2097259-has-config-schema-13.patch queued for re-testing.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as per @Gábor Hojtsy at #14

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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