Search button in Simple configuration overview (Config Entity) doesn't work.

When the source is of type SIMPLE_CONFIG we can not use the search form.
Here is a patch for that and a test.

+++ b/sources/tmgmt_config/src/Tests/ConfigSourceListTest.php
@@ -234,6 +234,9 @@ class ConfigSourceListTest extends EntityTestBase {
+    // Assert no option to search.
+    $this->assertNoFieldById('edit-search-submit', 'Search');
+

Extend this test, add coverage for searches with results and no results. Try to test that results are shown in /admin/tmgmt/sources/locale/default/ as expected and so on and so forth.

The rest looks good to me.

I think we can create a follow up for a better test coverage since this issue is about the broken button. I am not sure because some of the entities have already a test filter including LocaleSourceUiTest that tests filters in /admin/tmgmt/sources/locale/default/

This looks correct, removing the search button and adding a test to see if it is removed seems sufficient for this issue.

Hm, actually, at least some of those filters are still useful, for example searching on the name, and also target language but that's likely not implemented atm.

Added filters for name and target language and also test coverage for them.

+++ b/sources/tmgmt_config/src/ConfigSourcePluginUi.php
@@ -249,13 +249,37 @@ class ConfigSourcePluginUi extends SourcePluginUiBase {
-

Why removing empty lines? :)

+++ b/sources/tmgmt_config/src/ConfigSourcePluginUi.php
@@ -249,13 +249,37 @@ class ConfigSourcePluginUi extends SourcePluginUiBase {
+          // Show translated Config entities.
...
+          // Show untranslated Config entities.

I don't think we need to use "Config entities" with capital C.

Other than that, seems fine.

Ups, I didn't realize that. Fixed now.

+++ b/sources/tmgmt_config/src/Tests/ConfigSourceListTest.php
@@ -286,6 +286,28 @@ class ConfigSourceListTest extends EntityTestBase {
+    $this->drupalGet('admin/tmgmt/items/1');
+    $this->drupalPostForm(NULL, array(), t('Save as completed'));

i think you can put this together.

The rest looks good to me.

Done.

+++ b/sources/tmgmt_config/src/ConfigSourcePluginUi.php
@@ -254,8 +254,33 @@ class ConfigSourcePluginUi extends SourcePluginUiBase {
       $definitions = \Drupal::service('plugin.manager.config_translation.mapper')->getDefinitions();
+      $definitions_filter = array();
+      // Filtering by title first to have a better order.
       foreach ($definitions as $definition_id => $definition) {
         if (!isset($definition['entity_type'])) {
+          if (!isset($search_property_params['name'])) {
+            $definitions_filter[$definition_id] = $definition;
+          }
+          elseif (strpos(strtolower($definition['title']), strtolower($search_property_params['name'])) !== FALSE) {

The logic of the filtering is quite repetitive here at the moment.

I'd suggest you add a $this->getFilteredSimpledConfigDefinitions($search_property_params) method here.

Then in there, you can just have a single loop with a list of checks, if any doesn't match you just use continue; to skip to the next definition.

Using array_filter() with a callback or anonymous function would be another option.

I added a function and instead of using a loop I am using an array filter. Looks better now.

+++ b/sources/tmgmt_config/src/ConfigSourcePluginUi.php
@@ -415,4 +413,48 @@ class ConfigSourcePluginUi extends SourcePluginUiBase {
+    $definitions = array_filter($definitions, function ($definition) use ($search_properties) {
+      if (!isset($definition['entity_type'])) {
...
+    if (isset($search_properties['target_language'])) {
...
+      $definitions = array_filter($definitions, function ($definition) use ($target_status, $target_language) {

We are doing the same thing here - filtering. Can we combine these 2 loops into one and turn the logic a bit around?

E.g.

<?php
if (isset($definition['entity_type']) || !isset($search_properties['target_language'])) { 
   return FALSE; 
}
?>

etc.

Now both filters in the same loop and the logic changed a bit.

Patch looks good, works as intended. Maybe take a look at this issue which is quite similar.

+++ b/sources/tmgmt_config/src/Tests/ConfigSourceListTest.php
@@ -286,6 +286,27 @@ class ConfigSourceListTest extends EntityTestBase {
+    $filters = array(
+      'search[target_language]' => 'de',
+      'search[target_status]' => 'translated',
+    );
+    $this->drupalPostForm('admin/tmgmt/sources/config/_simple_config', $filters, t('Search'));
+
+    // Just 1 simple configuration was translated.

Can you add another test that tests for both the name and the status?

I have a feeling that's not going to work as expected with the current code..

Added tests when target status is 'translated' and when is not.

+++ b/sources/tmgmt_config/src/Tests/ConfigSourceListTest.php
@@ -286,6 +286,48 @@ class ConfigSourceListTest extends EntityTestBase {
+    // Check if the list has 2 rows.
+    $this->assertEqual(count($this->xpath('//tbody/tr')), 2);
...
+    // Just 1 simple configuration was translated.
+    $this->assertEqual(count($this->xpath('//tbody/tr')), 1);
...
+    // There is 1 simple configuration untranslated with name 'settings'.
+    $this->assertEqual(count($this->xpath('//tbody/tr')), 1);
...
+    // There are 2 simple configurations with name 'sys' but just 1 is translated.
+    $this->assertEqual(count($this->xpath('//tbody/tr')), 1);

I'd put the comment as the third argument, you then get pretty information in the test and save four lines! :D
Maybe shorten the comments a little:
Just 1 simple configuration was translated. -> Found one translated simple configuration

If the count is not equal to one, the test tells you it failed finding a single configuration etc.

Sascha also asked you to add a test for this:

Can you add another test that tests for both the name and the status?

And while you have a lot of scenarios covered, exactly that one is missing (and broken).

Query: Valid HTML found on "http://d8.dev/admin/tmgmt/sources/config/_simple_config?name=system&langcode=&target_language=&target_status=translated"

Output:

The first entry is not translated, why is it displayed?

It is not possible to filter without target_language, so the patch was quite fine (working as intended) but here is a much better approach.

Nice, committed.