QueryBase::sort can only work with capital letter such as 'ASC' or 'DESC' as the second parameter

I would suggest a patch since QueryBase::tableSort() is calling tablesort_get_sort() and then sort(); tablesort_get_sort() returns lowercase 'asc' or 'desc', and one of those is passed to sort(). tablesort_get_sort() should return uppercase 'ASC or 'DESC', but historically the function looks returning a lowercase value. By considering that, the parameter 'asc' or 'desc' to sort() can be converted to uppercase inside sort() function.

tablesort API page: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

@dawehner

I tried to use both EntityListBuilder and ConfigListBuilder. The problem here is that tableSort function doesn't work without converting uppercase. Maybe I am wrong to use *ListBuilder but after my deep investigation, I needed to pass an uppercase parameter value such as 'ASC' and 'DESC' to sort() function. When we grep entire Drupal 8 Core code, we can find it includes mixed upper- or lower-case for 'ASC' or 'DESC'. I think it causes an incompatible parameter value issue among the internal function calls. What I identified is that there has a problem in between tablesort_get_sort and sort in tableSort. If sort function can internally convert a parameter value to uppercase like my patch, we can write the following code:

Before:

  $header  = $this->buildHeader();
  $storage = $this->getStorage();
  $query   = $storage->getQuery();
  $order   = tablesort_get_order($header);
  $sort    = \Drupal\Component\Utility\Unicode::strtoupper(
             tablesort_get_sort($header));
  $keys    = $query->tableSort($header)
                   ->sort($order, $sort)
                   ->execute();

After:

  $header  = $this->buildHeader();
  $storage = $this->getStorage();
  $query   = $storage->getQuery();
  $keys    = $query->tableSort($header)
                   ->execute();

Thus, "After" is what I expect as DX.

@dawehner

Uh... I am sorry I think I missed your point. I thought that we should always use Drupal-defined functions for code consistency. I felt \Drupal\Component\Utility\Unicode... was redundant. Let me put a patch to simply this with strtoupper().

@jhodgdon

After my one hour debugging core code, now I can clearly answer to your question by attaching a new patch. (sorry my previous patch anyhow couldn't passed the testing due to non "unix-style line endings".)

Here is my findings:

- A sort() method simply returns an array. (Therefore it is not a candidate of the root cause.)
- tableSort _does_ work for ContentEntity (no case-sensitve) but doesn't work in case of ConfigEntity.
- An implementation of execute() method is totally different in between ContentEntity and ConfigEntity (see below to compare).

1) ContentEntity - Drupal\Core\Entity\Query\Sql\Query::execute()

public function execute() {
  return $this
  ->prepare()
    ->compile()
    ->addSort()
    ->finish()
    ->result();
}

2) ConfigEntity - Drupal\Core\Config\Entity\Query\Query::execute()

public function execute() {

  // ... (snip) ...

  // Apply sort settings.
  foreach ($this->sort as $sort) {
    $direction = $sort ['direction'] == 'ASC' ? -1 : 1;
    $field = $sort ['field'];
    uasort($result, function($a, $b) use ($field, $direction) {
      return ($a [$field] <= $b [$field]) ? $direction : -$direction;
    });
  }
  // .. (snip) ...
}

I think strtoupper() is required for the following code in a variable $sort ['direction'].

  $direction = $sort ['direction'] == 'ASC' ? -1 : 1;

PS Thank you for your advice to interact this issue queue.

@daffie

I am trying to create test cases based on Drupal\config\Tests\ConfigEntityListTest for tabeSort function in ConfigEntity but I am facing an issue because ConfigTestListBuilder::buildHeader() method doesn't have a data structure w/ sorting. The following is the difference.

Current:

  public function buildHeader() {
    $header['label'] = t('Label');
    $header['id'] = t('Machine name');
    return $header + parent::buildHeader();
  }

Expected:

  public function buildHeader() {
    $header = array(
      array('data' => t('Label'), 'specifier' => 'label'),
      array('data' => t('Machine name'), 'specifier' => 'id'),
    );
    return $header + parent::buildHeader();
  }

When I modify from the current to expected one, it affects existing test cases. I tried to put a flag to switch in between the current and expected ones (above) but It looks we cannot change the header as long as we use the same config_test entity. Once SimpleTest starts, it looks we cannot change the buildHeader ($header + parent::buildHeader()). (I am not sure if it is possible or not but I couldn't make it although I tried a variety of angles) How can I change the table header with or without sorting attributes inside Drupal\config\Tests\ConfigEntityListTest class?

I could create my test cases based on the expected data structure above but it will change the existing test cases such as:

- ConfigEntityListTest::testList()
- ConfigEntityListTest::testListUI()
- ConfigEntityListTest::testPager()

Anyhow, to change buildHader() method will affect the existing test case implementation. Please advise us about how we can take an approach as follows:

1. Based on the existing 'config_test' entity, include TableSort header data structure with the existing test cases at ConfigEntityListTest (Major change will happen in ConfigEntityListTest) and will lose non-tablesort tests. _OR_
2. Create a new ConfigEntity other than 'config_test' (e.g. config_test_tablesort)

I think we need to test with or without tableSort function. So my question is: How we can handle both test requirements (with or without tableSort) in one ConfigEntityListTest?

Finally I came up with a test by extending Drupal\config\Tests\ConfigEntityListTest. I tried to extend the existing test code w/ minimum addition/modification. Actually I added a testTableSort() method to ConfigEntityListTest class.

I am attaching two patches as @daffie suggested:

1. The first with only the test and that should fail the testbot.
2. The second patch with the test and the solution and that one should pass.

@dawehner

It might be virtually enough to add a test code 'testTableSort' in core/modules/system/src/Tests/Entity/ConfigEntityQueryTest.php as the attached patch, however still it tests the following code. For example:

  $this->queryResults = $this->factory->get('config_query_test')
    ->tableSort($header)
    ->sort('id', 'DESC') // 'DESC' upper case
    ->execute();

However, for DX, we would like to test without sort function as follows:

  $this->queryResults = $this->factory->get('config_query_test')
    ->tableSort($header)
    ->execute();

In order to test the latter one, I thought we needed to modify the buildHeader in core/modules/config/tests/config_test/src/ConfigTestListBuilder.php. Then, back to my comment #13, I couldn't find any sophisticated solution to add/modify the existing test cases, that's why I introduced core/modules/config/tests/config_test/src/ConfigTestTableSortListBuilder.php

Sorry, comments in the patches were not correct. Please let me re-submit the patches.

@daffie

1. Moved the comments, please review.

2. For the last 4 tests, the error was "Link Label does not exist on http://localhost/checkout/admin/structure/config_test_table_sort". It looks beyond strtoupper tests. It has some table header and the link problem? I don't know why because it was successful to test on my local D8 installation.