Fix deprecations including jQuery UI dependency and info.yml update

Thanks for the patch.

1) Reviewed the code:

• +++ b/composer.json
  @@ -6,5 +6,8 @@
  +  "require": {
  +    "drupal/jquery_ui_accordion": "^1.0"
  +  },
  

  Not part of the change record instructions and is optional but ok to include per @berdir in #d9readiness Slack:

  https://drupal.slack.com/archives/CDDD98AMN/p1585592660080800?thread_ts=...

  changing composer.json is optional, it's enough to just change .info.yml if the module has no composer.json/no drupal/core requirement

• +++ b/tests/src/Functional/ViewsAccordionTest.php
  @@ -14,6 +14,11 @@ class ViewsAccordionTest extends BrowserTestBase {
  +  protected $defaultTheme = 'stark';
  +
  +  /**
  +   * {@inheritdoc}
  +   */
  

  This was added to makes tests pass due to: #3082655: Specify the $defaultTheme property in all functional tests

• +++ b/tests/src/FunctionalJavascript/ViewsAccordionTest.php
  @@ -16,6 +16,11 @@ class ViewsAccordionTest extends WebDriverTestBase {
  +  protected $defaultTheme = 'stark';
  +
  +  /**
  +   * {@inheritdoc}
  +   */
  

  Ditto.

• +++ b/views_accordion.info.yml
  @@ -4,3 +4,6 @@ description: 'Provides an accordion views display plugin.'
  +  - jquery_ui_accordion:jquery_ui_accordion
  +test_dependencies:
  +  - jquery_ui_accordion:jquery_ui_accordion
  

  Looks good. Step 2 in change record: https://www.drupal.org/node/3067969

• +++ b/views_accordion.libraries.yml
  @@ -3,4 +3,4 @@ views_accordion.accordion:
  -    - core/jquery.ui.accordion
  +    - jquery_ui_accordion/accordion
  

  Looks good. Step 3 in change record: https://www.drupal.org/node/3067969

2) Patch applied cleanly:

[mac:kristen:views_accordion]$ patch -p1 < 3091388-6.patch 
patching file composer.json
patching file tests/src/Functional/ViewsAccordionTest.php
patching file tests/src/FunctionalJavascript/ViewsAccordionTest.php
patching file views_accordion.info.yml
Hunk #1 succeeded at 5 with fuzz 1 (offset 1 line).
patching file views_accordion.libraries.yml

Note that I applied the patch on top of info file patch to see if they were compatible: https://www.drupal.org/project/views_accordion/issues/3100388#comment-13...

3) Reviewed Slack discussion regarding the new code breaking existing sites

https://app.slack.com/client/T06GX3JTS/CDDD98AMN/thread/CDDD98AMN-158806...

From @berdir:

you could include an update function to enable the new dependency automatically. anyone with composer will get the new dependency automatically and others should get an error on update.php about the missing dependency

and

new major version or not is up to you I guess

From @penyaskito:

depending on jquery_ui_accordion already forces you to use 8.7.7 or higher, as it doesn’t define core:8.x

Based on the above comments, adding the update hook is optional and doesn't have to hold up this patch.

4) Searched the code for jquery.ui.accordion after applying the patch and found it in:

./views_accordion.module:      $output .= '<h4>' . t('Choose <em>Views Accordion</em> in the <em>Style</em> dialog within your view, which will prompt you to configure the jquery.ui.accordion settings.') . '</h4>';

In the jQuery UI Accordion README file, it says:

2. Change any references in your theme or module from
`core/jquery.ui.accordion` to `jquery_ui_accordion/accordion`

so, although a nitpick, IMO this should be changed so moving back to "Needs work".

5) Other than #4, this looks RTBC to me but needs some manual testing for <8.7.7, 8.8, and 9.

Whoops :)

Hi @kristen-pol,

Thanks for reviewing the patch. I have gone through your suggestions and accordingly fixed the 4th point and updated the patch.

Please review.

Hi @kristen-pol,

Thanks for reviewing the patch.

Please ignore the previous comment. Submitted the wrong patch file.

I have gone through your suggestions and accordingly fixed the 4th point and updated the patch 3091388-13.patch

Please review.

Fixing this:

1) Drupal\Tests\views_accordion\Functional\ViewsAccordionTest::testViewsAccordion
Drupal\Core\Extension\InfoParserException: The 'core_version_requirement' key must be present in modules/contrib/views_accordion/tests/modules/views_accordion_test/views_accordion_test.info.yml

Fixing this:

Drupal\Tests\views_accordion\FunctionalJavascript\ViewsAccordionTest::testViewsAccordion
Using assertContains() with string haystacks is deprecated and will not be supported in PHPUnit 9. Refactor your test to use assertStringContainsString() or assertStringContainsStringIgnoringCase() instead.
Using assertNotContains() with string haystacks is deprecated and will not be supported in PHPUnit 9. Refactor your test to use assertStringNotContainsString() or assertStringNotContainsStringIgnoringCase() instead.

Looks like all remaining deprecations are being fixed here so I think we should change the title of the issue to reflect that but will see if @Manuel Garcia would like to weigh in on that.

Since the info.yml file fix is getting moved to here, adding the related issues but not closing it out yet until it's clear that this issue will handle all deprecations. If this will include the info.yml file, please add an issue credit here for @katherined.

Related issues:

#3100388: Drupal 9 compatibility
#3042793: Drupal 9 Deprecated Code Report

Took a stab at renaming the title.

I checked the interdiffs for #13, #17, #18, and #19 and they all seem ok.

At this point, I think it's just manual testing that's needed.

Tested patch #19 on 9.1.0-dev. I'm seeing an error on console. Looks like there is still some dependency on jquery.
Attaching the screenshot.

My Bad. Patch was not applied correctly on 9.1.0-dev. Tested again and works fine on 9.1.0-dev and ^8.8.x

@mohrerao Thanks for testing on 8.8 and 9.1. Would you be able to follow the steps in the issue summary for Drupal <8.7.7? We want to make sure this update isn't going to break any existing sites.

@kristen, 3091388-19.patch cannot be tested on versions below 8.6 as we have addded core_version_requirement: ^8.7.7 || ^9 in the info.yml

Good point. I'm curious how it would complain if someone tried it on an older site though.

I see the failure in the tests for PHP 7 & MySQL 5.5, D8.7.

@Manuel Garcia Do you think this has been tested enough?

The better option would be to create a new version for D9 so that D8 sites can continue using the current version.

@mohrerao There are maintainers that are doing that but many are making their D8 version compatible with D9. IMO you are correct and we don't need to test with < 8.7.7 because composer won't install this version of the module if they are on 8.7.7+.

Marking RTBC and @Manuel Garcia can chime in if this is enough testing.

Use event-appropriate tag (based on DIACWMay2020). (Sorry for the noise).

Manually tested against 9.0.x-dev - seems to work as expected.

Ran against drupal-check, shows no deprecations.

Patch #36 looks fine as well :)

#36, refactoring it is good workaround. I've worked on another similar one, #3126787: [D8 only] Add forwards-compatibility shim for assertInternalType() replacements in phpunit 6&7, there is no chance in core backporting them to 8.7.7 from my understanding. even It took some extra time of committers to make the decision backported them to 8.8.x finally.

+++ b/views_accordion.module
@@ -23,7 +23,7 @@ function views_accordion_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<h4>' . t('Choose <em>Views Accordion</em> in the <em>Style</em> dialog within your view, which will prompt you to configure the jquery_ui_accordion settings.') . '</h4>';

Perhaps, use "jQuery UI Accordion" is more applicable here, instead of machine name "jquery_ui_accordion", this can be fixed on committing if necessary.

Setting back to RTBC.