Problem

Libraries API module is supported by Paragraphs to optionally load "Sortable" library.

Libraries API has changed the approach used in Drupal 8, so that a number of functions are deprecated there including the one used by Paragraphs. However neither the documentation page nor the code of Libraries API explains the up-to-date way to detect libraries.

We should avoid using deprecated functions to make Paragraphs D9-ready.

Proposed solutions

Figure out the up-to-date way to use Libraries API and implement it in Paragraphs.

OR

Remove Libraries API support and use the fallback option already implemented in Paragraphs. (It might make sense since Libraries API looks abandoned for Drupal 8.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Alex Bukach created an issue. See original summary.

Berdir’s picture

See #3090457: Use sortable library from core, combined with requiring 8.8, we can simply remove that now.

Berdir’s picture

Title: Update libraries integration » Remove own sortablejs library, remove remaining 8.8 deprecations
Parent issue: #3042592: Remove Drupal 8.7 and earlier deprecations »
Related issues: +#3042592: Remove Drupal 8.7 and earlier deprecations
sidharthap’s picture

Status: Active » Needs review
FileSize
3.97 KB

This patch removes own sortablejs library. tested the Drag & drop functionality with core sortable library.

pstewart’s picture

In backporting 3055618-4.patch to apply it to 1.10 I ran into a problem with hook_library_info_alter not behaving as expected. Looking at the code block removed, the function returns libraries unchanged for D8.8, so the entire alter hook can actually be removed given that this patch enforces D8.8. This worked for me when patching 1.10

Berdir’s picture

Status: Needs review » Needs work

That is correct.

That said, it might be easier to use the path from #3090457: Use sortable library from core, which will dynamically use the library from core already.

sidharthap’s picture

Status: Needs work » Needs review
FileSize
5.25 KB

@pstewart , @Berdir Thank you. I tested the drag and drop functionality on a new setup after removing alter hook and its working.

Berdir’s picture

Status: Needs review » Needs work

Yes, that looks good so far, now we also need to take care of the remaining 8.8 deprecations like entity_get(_form)_display().

sidharthap’s picture

Status: Needs work » Needs review
FileSize
18.67 KB
sidharthap’s picture

small correction.

Berdir’s picture

@sidharthap: Thanks, please always provide an interdiff when making changes to an existing patch, so that it is easy to see what changed: https://www.drupal.org/documentation/git/interdiff

Berdir’s picture

Status: Needs review » Needs work

Still a few things left to do:

  21x: Any entity_reference_autocomplete component of an entity_form_display must have a match_limit setting. The field_reusable_paragraph field on the paragraph.from_library.default form display is missing it. This BC layer will be removed before 9.0.0. See https://www.drupal.org/node/2863188
  2x: Any entity_reference_autocomplete component of an entity_form_display must have a match_limit setting. The uid field on the node.paragraphed_content_demo.default form display is missing it. This BC layer will be removed before 9.0.0. See https://www.drupal.org/node/2863188

You need to add match_limit: 10 to core.entity_form_display.paragraph.from_library.default.yml below match_operator. And same for some other files.

  2x: Support for asserting against non-boolean values in ::assertTrue is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use a different assert method, for example, ::assertNotEmpty(). See https://www.drupal.org/node/3082086
    1x in ParagraphsTypesTest::testParagraphTypeDefaultIcon from Drupal\Tests\paragraphs\Functional\Classic
    1x in ParagraphsExperimentalBehaviorsTest::testBehaviorPluginsSettingsFiltering from Drupal\Tests\paragraphs\Functional


  1x: Support for asserting against non-boolean values in ::assertFalse is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use a different assert method, for example, ::assertEmpty(). See https://www.drupal.org/node/3082086
    1x in ParagraphsExperimentalBehaviorsTest::testBehaviorPluginsSettingsFiltering from Drupal\Tests\paragraphs\Functional

For these, you need to find the matching assertTrue/False that are not actually checking for a boolean value but a string or so and change it accordingly.

  1x: \Drupal\Tests\PhpunitCompatibilityTrait:setExpectedException() is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Backward compatibility for PHPUnit 4 will no longer be supported. See https://www.drupal.org/node/3056869
    1x in FieldCollectionsFieldInstanceSettingsTest::testFieldCollectionBadBundle from Drupal\Tests\paragraphs\Unit\migrate

See message, I think this is just expectedDeprecation() now.

sidharthap’s picture

Status: Needs work » Needs review
FileSize
43.51 KB
26.07 KB

Thank you @Berdir.
Updated patch without setExpectedException().

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/modules/paragraphs_library/tests/src/Functional/ParagraphsLibraryTest.php
    @@ -360,7 +360,7 @@ class ParagraphsLibraryTest extends ParagraphsExperimentalTestBase {
         $third_party = $config_factory->get('paragraphs.paragraphs_type.text')->get('third_party_settings');
    -    $this->assertFalse(isset($third_party['paragraphs_library']['allow_library_conversion']));
    +    $this->assertNull(isset($third_party['paragraphs_library']['allow_library_conversion']));
     
    

    isset() returns true or false, so that was correct. You need to be careful to only change the lines that were actually not a boolean.

  2. +++ b/modules/paragraphs_library/tests/src/FunctionalJavascript/ParagraphsLibraryItemEntityBrowserTest.php
    @@ -133,7 +133,7 @@ JS;
         $rows = $this->xpath('//*[@id="entity-browser-paragraphs-library-items-form"]/div[1]/div[2]/table/tbody/tr');
    -    $this->assertTrue(count($rows) == 1);
    +    $this->assertEquals(1, count($rows));
    

    same here and many others. This could use assertCount() or just don't change it.

sidharthap’s picture

Status: Needs work » Needs review
FileSize
9.54 KB
45.27 KB

Thank you @Berdir. Updated patch

Berdir’s picture

Status: Needs review » Needs work
+++ b/modules/paragraphs_library/tests/src/Functional/ParagraphsLibraryTest.php
@@ -360,7 +360,7 @@ class ParagraphsLibraryTest extends ParagraphsExperimentalTestBase {
     $third_party = $config_factory->get('paragraphs.paragraphs_type.text')->get('third_party_settings');
-    $this->assertFalse(isset($third_party['paragraphs_library']['allow_library_conversion']));
+    $this->assertEmpty(isset($third_party['paragraphs_library']['allow_library_conversion']));
 
     $this->drupalGet('node/add/paragraphed_test');

+++ b/modules/paragraphs_library/tests/src/FunctionalJavascript/ParagraphsContentModerationTest.php
@@ -184,7 +184,7 @@ class ParagraphsContentModerationTest extends WebDriverTestBase {
     $host_node = $this->getLastEntityOfType('node', TRUE);
     $host_node_id = $host_node->id();
-    $this->assertFalse($host_node->access('view', $this->visitorUser));
+    $this->assertEmpty($host_node->access('view', $this->visitorUser));
 

@@ -270,7 +270,7 @@ class ParagraphsContentModerationTest extends WebDriverTestBase {
     $page->pressButton('Save');
     // The admin is currently at /node/*/latest.
-    $this->assertTrue(strpos($session->getCurrentUrl(), "/node/{$host_node_id}/latest") !== FALSE);
+    $this->assertNotEmpty(strpos($session->getCurrentUrl(), "/node/{$host_node_id}/latest") !== FALSE);
     $assert_session->pageTextContains('paragraphed_moderated_test Host page 1 (rev 4) has been updated.');
     // The admin user should be seeing the latest, forward-revision.
     $assert_session->pageTextNotContains('Direct paragraph text 3');
@@ -434,8 +434,8 @@ class ParagraphsContentModerationTest extends WebDriverTestBase {

Still 3 test fails and most calls are not correctly changed.

Only checked the first 3 examples, but you can see that they all return true/false only, so assertTrue/False is the correct thing to do here and they should not be changed at all.

sidharthap’s picture

Yes @Berdir, assertTrue/False should not be changed in case bool. reverted such cases.

Berdir’s picture

Status: Needs review » Needs work

We're getting close here, did run the test on D9 locally and while a few still fail due to dependencies, most passed, only non-dependency related error was this:

1) Drupal\Tests\paragraphs\Unit\migrate\FieldCollectionsFieldInstanceSettingsTest::testFieldCollectionBadBundle
Error: Call to undefined method Drupal\Tests\paragraphs\Unit\migrate\FieldCollectionsFieldInstanceSettingsTest::setExpectedException()

sidharthap’s picture

Status: Needs work » Needs review
FileSize
888 bytes
31.23 KB

Thank @Berdir.
Patch to remove setExpectedException.

Berdir’s picture

Status: Needs review » Fixed

And committed! Paragraphs 8.x-1.x-dev now requires Drupal 8.8 and should be fully compatible with Drupal 9.

  • Berdir committed a4f6f0c on 8.x-1.x authored by sidharthap
    Issue #3055618 by sidharthap: Remove own sortablejs library, remove...

Status: Fixed » Closed (fixed)

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

jonathanshaw’s picture

Looks like you forgot to bump core requirement in composer.json: #3123420: Bump core requirement in composer.json