AJAX error when switching between Content and Locale dropdown

We need to whitelist search options in all the places where we dynamically apply conditions:

You can reproduce this in even easier ways:

admin/tmgmt/sources/content/node?whatever=breakit

That will fail. Likely the same for config entities.

Use a whitelist array for all the allowed filter conditions then array_intersect() to limit to those.

Too complicated, just have a hardcoded whitelist, something like protected $conditionFields = [...], add in the dynamic field names while building the form.

+++ b/sources/content/src/ContentEntitySourcePluginUi.php
@@ -214,7 +214,7 @@ class ContentEntitySourcePluginUi extends SourcePluginUiBase {
       // Skip exclude params, and those that have empty values, as these would
       // make it into query condition instead of being ignored.
-      if (in_array($key, $exclude_params) || $value === '') {
+      if (!(in_array($key, $exclude_params) || $value == '')) {
         continue;
       }
       $search_property_params[$key] = $value;

I don't think this is doing what it should, but you likely did find the correct place.

However, based on what we found, we need to turn this pattern around. So instead of an exclude list, we need to have an include list/whitelist with allowed params and make sure the keys are in that.

1. +++ b/sources/content/src/ContentEntitySourcePluginUi.php
   @@ -208,20 +208,22 @@ class ContentEntitySourcePluginUi extends SourcePluginUiBase {
   +    // Load search property params which will be passed into.
   +    $whitelist = array(
   +      'title',
   

   Some of those fields are dynamic, specifically the title and bundle filter. it's title for node but name for terms, and it's type for nodes (missing here) but vid for terms.

   That's why the tests are failing. See the code in the search form, you need the same logic here. (getKey()).

2. +++ b/sources/content/src/ContentEntitySourcePluginUi.php
   @@ -208,20 +208,22 @@ class ContentEntitySourcePluginUi extends SourcePluginUiBase {
        $search_property_params = array();
   -    $exclude_params = array('q', 'page');
        foreach ($_GET as $key => $value) {
   -      // Skip exclude params, and those that have empty values, as these would
   -      // make it into query condition instead of being ignored.
   -      if (in_array($key, $exclude_params) || $value === '') {
   +      if ($value == '') {
            continue;
          }
          $search_property_params[$key] = $value;
        }
   -
   +    $search_property_params = array_intersect_key($search_property_params, array_flip($whitelist));
   

   This can actually be simplified into $search_property_params = array_filter($_GET). While touching this, let's switch to \Drupal::request()->query->all().

Oh and... needs tests :)

Just simulate an ajax request by mixing a valid filter argument with a non-valid one and make sure it has been filtered correctly.

1. +++ b/sources/content/src/ContentEntitySourcePluginUi.php
   @@ -208,18 +208,13 @@ class ContentEntitySourcePluginUi extends SourcePluginUiBase {
   +    array_push($whitelist, $entity_type->getKey('bundle'));
   +    array_push($whitelist, $entity_type->getKey('label'));
   

   $whitelist[] = $entity->getKey('ENTITY_KEY') is easier to read and does exactly the same in this situation. If you don"t need the return value of array_push (it returns the amount of elements in the array after the push), always use the $array[] way. [See also: http://php.net/manual/en/function.array-push.php#83388]

2. +++ b/sources/content/src/Tests/ContentEntitySourceListTest.php
   @@ -292,4 +292,21 @@ class ContentEntitySourceListTest extends EntityTestBase {
   +    $this->drupalGet('/admin/tmgmt/sources/content/node',[], $edit);
   

   Isn't this switched around? The third parameter is headers, the second is options. The options are used to build the Url.

1. +++ b/sources/content/src/ContentEntitySourcePluginUi.php
   @@ -208,18 +208,13 @@ class ContentEntitySourcePluginUi extends SourcePluginUiBase {
   +    // Load search property params which will be passed into.
   +    $entity_type = \Drupal::entityManager()->getDefinition($type);
   +    $whitelist = array('langcode', 'target_language', 'target_status');
   +    $whitelist[] = $entity_type->getKey('bundle');
   +    $whitelist[] = $entity_type->getKey('label');
   +    $search_property_params = array_filter(\Drupal::request()->query->all());
   +    $search_property_params = array_intersect_key($search_property_params, array_flip($whitelist));
   

   This looks pretty good now, just some minor improvements:

   a) The documentation isn't really clear to me. It was weird before and it's worse now. Suggestion:

   // Build a list of allowed search conditions and get their values from the request.

   Also, I still think we need the same check config entities or do they work differently?

2. +++ b/sources/content/src/Tests/ContentEntitySourceListTest.php
   @@ -292,4 +292,21 @@ class ContentEntitySourceListTest extends EntityTestBase {
   +
   +  /**
   +   * Tests that the term bundle filter works.
   +   */
   +  public function testAjaxFunctions() {
   

   I'd recommend to put this into one of the existing methods. The list tests are already very slow.

   The use of $term in the test method is also very confusing, $term is usually a taxonomy term. Just hardcode them into the string, should actually be easier to understand.

Yes, better now. Committed.