Sorting nodes by title aren't returned in correct order.

Thanks for reporting this bug and creating such a great issue summary! I was able to reproduce the issue when testing on 9.2 as well.

One thing I should have noted before is that I tried testing with specifying the langcode as en-US and also got a non-alphabetical ordering though I think it was different than in the issue summary. I might try that again.

JSON:API uses EFQ under the hood. See \Drupal\jsonapi\Controller\EntityResource::getCollectionQuery. One way to verify, that it's not a JSON:API issue, is to create an equivalent EFQ and check the results. JSON:API sort parameter is handled by \Drupal\jsonapi\Query\Sort and as per \Drupal\Tests\jsonapi\Unit\Query\SortTest::parameterProvider ?sort=title will be converted to following values

[[
  'path' => 'title', 
  'direction' => 'ASC', 
  'langcode' => NULL,
]]

You resultant EFQ will be:

$storage = \Drupal::entityTypeManager()
  ->getStorage('node');
$query = $storage
  ->getQuery()
  ->condition('type', 'article')
  ->sort('title', 'ASC', NULL);
$results = $query->execute();
$entities = $storage->loadMultiple($results);
foreach ($entities as $entity) {
 print $entity->title() . PHP EOL;
}

Can you please share the output of the above script?

@jibran
I am seeing different behavior.
I've attached a test only patch to see how it goes.

For me on local, entity queries are giving the correct results.
Somehow for my test case, when I request the translation URL like /ca/jsonapi/node/article, the sorting process is happening as per the english labels only.
Where as for translation sorting should happen as per translated titles.

Feel free to correct me if I am missing something here.

Can you try the following patch?

Merging the patches from #6 and #7

Hello, have just checked the patch #8, the list looks ordered properly, so I'm moving the issue to RTBC.

Please do not ask the testbot to try again until #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed.

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed. I have not reviewed the code.

1. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalMultilingualTest.php
   @@ -322,4 +322,64 @@ public function testDeleteMultilingual() {
      }
    
   +  /**
   +   * Test to evaluate the multilingual json api calls.
   +   */
   

   Should be 'Tests multilingual JSON:API calls.' I think.

2. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalMultilingualTest.php
   @@ -322,4 +322,64 @@ public function testDeleteMultilingual() {
   +  public function testMultilingualGet() {
   +    // Delete all the articles so we can start cleanly.
   +    $storage_handler = \Drupal::entityTypeManager()->getStorage('node');
   +    $entities = $storage_handler->loadByProperties(['type' => 'article']);
   +    $storage_handler->delete($entities);
   +
   +    $titles = [
   

   Should the article creation be moved out of ::setUp() into a helper method, so that we can avoid creating then deleting the nodes?

Should the article creation be moved out of ::setUp() into a helper method, so that we can avoid creating then deleting the nodes?

@catch, I tried to refactor it. However, currently we are creating articles like,
$this->createDefaultContent(5, 5, TRUE, TRUE, static::IS_MULTILINGUAL, FALSE); in setup() method.

Now, if we want to refactor it, we want to add a similar line in all the test cases of JsonApiFunctionalMultilingualTest class, so it will create the required nodes for all the test cases.

I found it a bit repetitive to do. Let me know if I should go ahead and refactor it.

@catch Updated the test cases as per discussion on slack and mentioned in #14

This issue also occurs on Drupal 8.9.12. Thanks @kuldeep_mehra27 #15 works!

The asked changes have been applies in #15 and #17, changed are minor. Setting it back to RTBC

+++ b/core/modules/jsonapi/src/Controller/EntityResource.php
@@ -176,8 +185,10 @@ class EntityResource {
+   * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
+   *   The language manager.
...
-  public function __construct(EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $field_manager, ResourceTypeRepositoryInterface $resource_type_repository, RendererInterface $renderer, EntityRepositoryInterface $entity_repository, IncludeResolver $include_resolver, EntityAccessChecker $entity_access_checker, FieldResolver $field_resolver, SerializerInterface $serializer, TimeInterface $time, AccountInterface $user) {
+  public function __construct(EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $field_manager, ResourceTypeRepositoryInterface $resource_type_repository, RendererInterface $renderer, EntityRepositoryInterface $entity_repository, IncludeResolver $include_resolver, EntityAccessChecker $entity_access_checker, FieldResolver $field_resolver, SerializerInterface $serializer, TimeInterface $time, AccountInterface $user, LanguageManagerInterface $language_manager) {

@@ -189,6 +200,7 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Ent
+    $this->languageManager = $language_manager;

We need to make this change in BC compatible way. I.e We need to allow NULLs for $language_manager and trigger a deprecation error if language manager is NULL. See \Drupal\Core\Extension\ModuleInstaller::__construct() on 9.3.x for a couple of examples of how to do this.

Thanks @alexpott
I've updated the changes and created a new fork for implementation.

I added test to test the deprecation message.

I wonder if we should be fixing this problem at a lower level. It seems to me that the result of calling ->sort() with a NULL in \Drupal\jsonapi\Controller\EntityResource::getCollectionQuery() is unexpected. Why is providing a langcode so important? Is it ever correct to provide a NULL when the field is not a base field?

I was wondering if there is something to be learned from this issue #2794431: [META] Formalize translations support in regards to this bug.

Thanks for the patch now we able to sort title

Left a comment and #26 also doesn't seem addressed. Setting to needs work.

As previous MR was unmergeable I created a new MR, where I applied the last patch (#17) and cherry picked the related-to-the-issue commits from the first MR.
Still needs work tough, to address the comment right before #30 by @bbrala

shouldn't this check for the entity type through the ResourceObject and make the language depend on what type of entiity this is?

And the question on #26 by @alexpott.

Edit: I believe I should have pushed the new branch before adding the commits, that way they would show here on the comments making clearer which commits I picked. So here is a copy of my log.

commit 0a900ba44c6effb16df6c0c440d3c1dffc3487d0 (cherry-picked from a281919c)
Author: Paulo Henrique Cota Starling <paulocs@ciandt.com>
Date:   Tue Aug 3 09:10:30 2021 -0300
    Changing to interface language instead of content language

commit c414f9b2b85014b083b53c83fc48c517e2036942 (cherry-picked from 9c55f6ad)
Author: Paulo Henrique Cota Starling <paulocs@ciandt.com>
Date:   Fri Jul 2 15:55:52 2021 -0300
    Adding 1 blank line after function

commit b6a6e57428840d7e784e5ccb54d7014e9c8475ea (cherry-picked from 0e15dffc)
Author: Paulo Henrique Cota Starling <paulocs@ciandt.com>
Date:   Fri Jul 2 13:58:01 2021 -0300
    Fix unknown word 'modulerenderer'

commit ee5a436dc307f43d8f6d17b36e5b4b98c9c098da (cherry-picked from 8f18c59d)
Author: Paulo Henrique Cota Starling <paulocs@ciandt.com>
Date:   Fri Jul 2 12:13:26 2021 -0300
    Adding test to the deprecation message

commit e5dab4de100b3a8f6600b016899268fc70bf9259
Author: andregp <andregp@ciandt.com>
Date:   Thu Apr 7 23:05:34 2022 -0300
    Applying patch #17

I created a patch for Drupal 10.0, if anybody else needs to patch this.
It's the same as #15 but without the tests.

Hi,

Current patches are not working with entities without translations (langcode column missing).

Column not found: 1054 Unknown column 'ENTITY.langcode' in 'on clause'.

- Combining both the PR's changes in single patch.
- Making it ready for 10.1.x's latest head
- Put back the original test cases from previous patches.

Addressing following feedback as well:

shouldn't this check for the entity type through the ResourceObject and make the language depend on what type of entiity this is?

Now, checking resource type and validating content/config entity. From there we are deciding the language type.

@cbccharlie
- I think it seems to be working fine for entities without translation. I tried to run `testRead()` in JsonApiFunctionalTest.
It worked fine for me.
Can you share if there are any specific things with your setup?
I tried following steps to validate the issue.
- Install standard profile.
- Enable jsonapi module
- Create a couple of page content
- Visit http://127.0.0.1:8080/jsonapi/node/page and I was able to see the results.

@alexpott
Can you please elaborate further #26
I am somewhat confused about it.

Fixing test case failures.
Ensuring that entity is translatable before passing the langcode.

@cbccharlie
I think this should fix the failures that you were experiencing.
Let me know if that helps.

Changes look good. Only think I can think of is a change record that should also be added to the trigger_error.

Added a change record here https://www.drupal.org/node/3357049
Updated the patch accordingly.

CR short and to the point.

Changes look good to me.

Triggering retest for #43 since failures seems random.

Left it back to RTBC per #44

Great work everyone! Some small nits ;)

+++ b/core/modules/jsonapi/src/Controller/EntityResource.php
@@ -170,14 +181,16 @@ class EntityResource {
-   * @param \Symfony\Component\Serializer\SerializerInterface|\Symfony\Component\Serializer\Normalizer\DenormalizerInterface $serializer
+   * @param \Symfony\Component\Serializer\SerializerInterface $serializer

I don't understand this change, and do not see the resoning on this in the comments. That worries me.

+++ b/core/modules/jsonapi/src/Controller/EntityResource.php
@@ -189,6 +202,11 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Ent
+    if (!$language_manager) {
+      @trigger_error('The language_manager service must be passed to ' . __NAMESPACE__ . '\EntityResource::__construct(). It was added in drupal:10.1.x and will be required before drupal:11.0.0. See https://www.drupal.org/node/3357049', E_USER_DEPRECATED);

Can we please be more explicit here? Perhaps:

if ($language_manager === NULL) {
...

This patch only addressing the second point mentioned by #48

Thanks, your interdiff does look weird though, you sure you interdiffed the right patches?

First feedback in my issue is indeed not addressed yet.

Should be moved to review when all points have been addressed or least attempted

Yes, created an interdiff between 43-49 patches

I've used the patch #43 and encountered an issue with it.

Setup

I have a D10.0.x site. It is multilingual (DE/FR).
I use the json api to get a filtered/sorted list of custom content entities.
The entity type has a translateable name and an untranslatable weight (number).

I request a list of the entities sorted by their weight (primary) and their name (secondary).
My requests look like this:
/LANGCODE/jsonapi/my_entity/my_entity?fields[my_entity--my_entity]=...&filter[status]=1&page[limit]=12&page[offset]=0&sort=field_weight,name

Problem

Not using the patch means that the name sorting doesn't work as expected for the French (non primary language) translations.
However, applying the patch causes a new issue for me.
Filtering by field_weight doesn't work anymore for French but it still works for German (primary/default language).
For French, I simply get a list sorted by name.

The cause appears to be the value in the langcode column of the my_entity__field_weight table.
I went into the database and changed the value to "und" this caused the sorting to also break for German. Updating the column again and setting the values back to 'de' fixed it again for German.

Thoughts

My issue is that the proposed solution seems to break sorting for untranslatable fields.
I guess this didn't happen before as the json api always used the default translation of the field for sorting.

I also guess that the json api is now always using the field value for the current language for sorting.
As the field has no specific value for French, it treats the field as if it where empty. The json api probably doesn't get that the field is not supposed to have a French value as it is not translatable.

Those are just my initial thoughts on this. I haven't had time to dive into the code of the json api and explore how sorting, etc. is implemented.

Please let me know if i understood something completely wrong and this is not unexpected behaviour.

Applying a patch with the changes mentioned on #48 comment in the #43 patch with the interdiff.

Re adding the files again with fixed paths.

Had the same issue mentioned in #54 with untranslatable fields.

Updated the patch so the langcode passes to the query only when the sort field is translatable.

This patch should pass the failed tests.

Patch for 11.x.

Tried to fix failure in #61.

Re-roll #59 for 10.2.x

Thanks for the fixes @xeniksp
I think test run should be green now.

Apart from your changes in the MR, I've addressed following thigns.

• Fixing test case failures.
• Fix the deprecation message formatting.
• Add the necessary trait for expectDeprecation method.
• Add additional test case to validate the sort order when sort happens on non translatable field. This case was raised in comment #54

Failing tests are passing on local now.

I'll move issue to Needs review after test run is successful.

Left some small comments.

CR appears to be outdated slightly.

Hiding patches for clarity.