Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
jQuery UI is being phased out from Drupal core: https://www.drupal.org/node/3064015
Deprecated unused jQuery UI asset libraries: https://www.drupal.org/node/3067969
Proposed resolution
We need to move to using https://www.drupal.org/project/jquery_ui_accordion instead of the core library to ensure a smooth transition to Drupal 9.
We need to ensure that when we do this, we require Drupal core 8.7.7 or later.
Remaining tasks
Write patch.Review it.- Manually test the upgrade ideally on actual sites,
Steps:- Install Drupal on each of these versions
- Drupal <8.7.7
- Drupal 8.8
- Drupal 9
- Enable views_accordion and configure a view to use it.
- Then apply the patch and report what happens:
- Install Drupal on each of these versions
- Commit it and tag new
2.0.0
release
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
@TODO
Comment | File | Size | Author |
---|---|---|---|
#36 | 3091388-36.patch | 6.67 KB | Manuel Garcia |
#36 | interdiff-3091388-19-36.txt | 3.65 KB | Manuel Garcia |
#29 | Screenshot 2020-05-13 at 1.52.33 PM.png | 481.42 KB | mohrerao |
#27 | Screenshot 2020-05-12 at 7.47.20 PM.png | 719.57 KB | mohrerao |
#19 | interdiff-3091388-18-19.txt | 2.98 KB | sja112 |
Comments
Comment #2
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #3
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #4
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedLet's try this again.
Comment #5
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #6
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedFinally progress, failure on #5 due to https://www.drupal.org/node/3083055
Comment #7
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #8
Kristen PolComment #9
Kristen PolComment #10
Kristen PolThanks for the patch.
1) Reviewed the code:
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=...
This was added to makes tests pass due to: #3082655: Specify the $defaultTheme property in all functional tests
Ditto.
Looks good. Step 2 in change record: https://www.drupal.org/node/3067969
Looks good. Step 3 in change record: https://www.drupal.org/node/3067969
2) Patch applied cleanly:
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:
and
From @penyaskito:
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:In the jQuery UI Accordion README file, it says:
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.
Comment #11
Kristen PolWhoops :)
Comment #12
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedHi @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.
Comment #13
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedHi @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.
Comment #14
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #15
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedI've updated the IS with steps to test this patch, which is the last thing we need to do in order to ensure a painless transition.
Also I have been thinking about it and in order to test the upgrade we need to change our core version requirement on to
core_version_requirement: ^8.7.7 || ^9
, so doing so in this patch.Comment #17
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedFixing this:
Comment #18
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedFixing this:
Comment #19
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedFixing this:
Comment #20
Kristen PolLooks 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
Comment #22
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedRe #20: I agree, makes sense to me to merge both into this.
Added issue credit to @katherined here.
Comment #23
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #24
Kristen PolTook a stab at renaming the title.
Comment #25
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedAh yes, title looks good thanks @Kristen Pol. Closed the other issue now.
Comment #26
Kristen PolI 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.
Comment #27
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedTested 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.
Comment #28
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #29
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedMy 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
Comment #30
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #31
Kristen Pol@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.
Comment #32
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@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
Comment #33
Kristen PolGood 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?
Comment #34
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedThe better option would be to create a new version for D9 so that D8 sites can continue using the current version.
Comment #35
Kristen Pol@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.
Comment #36
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedFirst of all, thank you @mohrerao very much for manually testing this, I believe with that, the version requirement and the dependency on the new
jquery_ui_accordion
contrib module it should be safe for this to go out.However, the current patch fails if tested against
8.7.x
, so the patch is not actually compatible with Drupal 8.7.7:I wonder if this situation is something core has a compatibility layer for, the failure is due core upgrading PHPUnit and although the module would work for the users, tests would fail for 8.7.7.
In any case, I had a think about this, and in order to move this forward I decided to refactor that part of the test in order to make it cross-version compatible, without using
assertContains
norassertStringContainsString
. Hopefully this will do the trick.Comment #37
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThere is talk on #3126797: [D8 only] Add forwards-compatibility shim for assertString(Not)ContainsString()replacements in phpunit 6&7 about what I worked around in #36 here.
Comment #38
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #39
Gábor HojtsyUse event-appropriate tag (based on DIACWMay2020). (Sorry for the noise).
Comment #40
Luke.LeberManually tested against 9.0.x-dev - seems to work as expected.
Ran against drupal-check, shows no deprecations.
Patch #36 looks fine as well :)
Comment #41
jungle#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.
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.
Comment #43
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedI have now created a new
2.0.x
branch and committed it.Thank you all again for the help on this, much appreciated!
Comment #44
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedhttps://www.drupal.org/project/views_accordion/releases/2.0.0