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

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.

The last submitted patch, 39: 3186834-39.patch, failed testing. View results →

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.

The last submitted patch, 43: 3186834-43.patch, failed testing. View results →

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 🐛 Sorting nodes by title aren't returned in correct order. Needs work comment in the #43 🐛 Sorting nodes by title aren't returned in correct order. Needs work patch with the interdiff.

Had the same issue mentioned in #54 🐛 Sorting nodes by title aren't returned in correct order. Needs work 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 for 10.2.x

mohit_aghera → changed the visibility of the branch 3186834-sorting-nodes-by--reroll to hidden.

mohit_aghera → changed the visibility of the branch 3186834-sorting-nodes-by to hidden.

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.