Problem/Motivation

The jQuery UI is being deprecated for Drupal 9 and therefore, we need to convert it to use the new SortableJS library as written in this deprecation page: jQuery UI Sortable deprecated and core usages removed. A quick search shows that it is listed as dependency for entity_reference and entity_reference.

Comments

sasanikolic created an issue. See original summary.

sasanikolic’s picture

Status: Active » Needs review
StatusFileSize
new4.61 KB

Here is the patch with the converted library to SortableJS.

Status: Needs review » Needs work

The last submitted patch, 2: 3110133-convert-sortable-library.patch, failed testing. View results

sasanikolic’s picture

Status: Needs work » Needs review
StatusFileSize
new5.27 KB
new898 bytes

Tests failed because a call to uniqueId(), which comes from jQuery UI which is removed and will be deprecated. Had to replace it with another unique ID generator, so I decided to use Crypto.getRandomValues(), as it seems like there's nothing like that in Drupal core's javascript that we could use. Let me know if there is a better solution to it.

Status: Needs review » Needs work

The last submitted patch, 4: 3110133-convert-sortable-library-4.patch, failed testing. View results

berdir’s picture

FWIW, uniqueId() isn't actually a _random_ id, it's just an incrementing counter: https://github.com/jquery/jquery-ui/blob/74f8a0ac952f6f45f773312292baef1...

This might be fine, but in case that makes it easier, we could also do something like that.

berdir’s picture

> 1) Drupal\Tests\entity_browser\FunctionalJavascript\EntityReferenceWidgetTest::testEntityReferenceWidget
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "entity.query". Did you mean one of these: "entity.query.config", "entity.query.sql", "pgsql.entity.query.sql", "entity.query.null", "entity.query.keyvalue"?

Noticed this one in the test fails, that's from:

$nid = $this->container->get('entity.query')->get('node')->condition('title', 'Referencing node 1')->execute();

I guess there's no @trigger_error() for the entity.query service and without this, we afaik don't even get far enough in the test to trigger it, so maybe just fix it here? just use \Drupal::entityQuery('node')->condition...

berdir’s picture

phenaproxima’s picture

Status: Needs work » Needs review
StatusFileSize
new9.59 KB

Here's some iterating on #4. Let's see if it helps with the D9 test failures.

Status: Needs review » Needs work

The last submitted patch, 9: 3110133-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

phenaproxima’s picture

Status: Needs work » Needs review
StatusFileSize
new11.97 KB
new2.82 KB

This should get EntityReferenceWidgetTest to pass; I modified it to use SortableTestTrait.

Status: Needs review » Needs work

The last submitted patch, 11: 3110133-11.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
StatusFileSize
new14.17 KB
new2.27 KB

Lucky #13. 😬😲🤭 This fixed the remaining failure for me locally; let's see how it fares.

oknate’s picture

StatusFileSize
new1.4 KB
new14.15 KB
new5.37 MB

As per Berdir's recommendation, replacing the crypto counter with a simple counter. I've added a video with the patch (with an additional debug statement to show the counting).

Other than that I think this is ready to go. Sorting seems to work fine.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

It looks like this module already fails tests against PostgreSQL, so this patch doesn't appear to be introducing problems there.

Given that, if it passes on MySQL for 8.8, 8.9, and 9.0, it's ready IMHO. I'm not really supposed to RTBC my own work, but who else will? So let's do this, on the presumption that those runs will come back green.

phenaproxima’s picture

Priority: Normal » Major
Issue tags: +Drupal 9 compatibility

This issue flat-out blocks Drupal 9 compatibility, since core no longer ships with jQuery UI. Therefore, I'm escalating its priority, and IMHO this should be critical (but I'll leave that decision to the maintainers).

  • oknate committed 188662a on 8.x-2.x
    Issue #3110133 by phenaproxima, sasanikolic, oknate, Berdir: Convert...
oknate’s picture

Status: Reviewed & tested by the community » Fixed

Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.