Fix remaining Drupal 10 compatibility issues [#3350661]

Problem/Motivation

There are still a few remaining Drupal 10 compatibility issues remaining, which should be resolved:

web/modules/contrib/drowl_admin/drowl_admin.libraries.yml:
┌─────────┬──────┬───────────────────────────────────────────────────────────┐
│ STATUS  │ LINE │                          MESSAGE                          │
├─────────┼──────┼───────────────────────────────────────────────────────────┤
│ Manuell │ 0    │ The 'admin_theme_overrides_gin' library is depending on a │
│ überpr� │      │ deprecated library. The core/jquery.once asset library is │
│ �fen    │      │ deprecated in Drupal 9.3.0 and will be removed in Drupal  │
│         │      │ 10.0.0. Use the core/once library instead. See            │
│         │      │ https://www.drupal.org/node/3158256                       │
│         │      │                                                           │
└─────────┴──────┴───────────────────────────────────────────────────────────┘

web/modules/contrib/drowl_admin/css/drowl_admin.layout_builder.claro.min.css:
┌─────────┬──────┬─────────────────────────────────────────────────────┐
│ STATUS  │ LINE │                       MESSAGE                       │
├─────────┼──────┼─────────────────────────────────────────────────────┤
│ Manuell │ 0    │ The #drupal-off-canvas selector is deprecated in    │
│ überpr� │      │ drupal:9.5.0 and is removed from drupal:10.0.0. See │
│ �fen    │      │ https://www.drupal.org/node/3305664.                │
│         │      │                                                     │
└─────────┴──────┴─────────────────────────────────────────────────────┘

web/modules/contrib/drowl_admin/css/drowl_admin.layout_builder.gin.min.css:
┌─────────┬──────┬─────────────────────────────────────────────────────┐
│ STATUS  │ LINE │                       MESSAGE                       │
├─────────┼──────┼─────────────────────────────────────────────────────┤
│ Manuell │ 0    │ The #drupal-off-canvas selector is deprecated in    │
│ überpr� │      │ drupal:9.5.0 and is removed from drupal:10.0.0. See │
│ �fen    │      │ https://www.drupal.org/node/3305664.                │
│         │      │                                                     │

Steps to reproduce

Proposed resolution

Fix the remaining issues.

Remaining tasks

User interface changes

API changes

Data model changes

Comment	File	Size	Author
#24	2023-07-13 12_18_42-Issue #3350661_ Fix remaining Drupal 10 compatibility issues (!6) · Merge reques.png	17.85 KB	Grevil

Issue fork drowl_admin-3350661

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

3350661-fix-remaining-drupal changes, plain diff MR !6
Check out this branch

About issue forks

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

27 March 2023 at 14:37

Grevil created an issue. See original summary.

Comment #2

27 March 2023 at 14:44

Grevil opened merge request !6

Comment #3

Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commented 27 March 2023 at 14:47

Assigned:	Unassigned	» thomas.frobieter
Status:	Active	» Needs work

OK, I fixed the remaining deprecated dependency. Now only the CSS issues remain, giving this issue to you @thomas.frobieter, all "#drupal-off-canvas" occurences should target "#drupal-off-canvas-wrapper" for D10 and "#drupal-off-canvas:not(.drupal-off-canvas-reset)" for Drupal 9, see: https://www.drupal.org/node/3305664.

@Anybody, since the first selector is Drupal 10 specific, I suggest we drop the Drupal 10 compatibility for the current branch and create a new 3.x branch which is D10 only.

Comment #4

Anybody

German

Porta Westfalica

CreditAttribution: Anybody at DROWL.de commented 27 March 2023 at 14:51

Thx @Grevil! I agree, please merge this into 2.x and remove || ^10 and create a new 3.x branch with the proposed changes as separate issue afterwards. Assigned to @thomas.frobieter.

PS: Crazy and super cool that Drupal detected this!

Comment #5

Anybody

German

Porta Westfalica

CreditAttribution: Anybody at DROWL.de commented 27 March 2023 at 14:53

Assigned:	thomas.frobieter	» Grevil
Status:	Needs work	» Needs review

Comment #6

Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commented 31 March 2023 at 08:58

Status:

Needs review

» Needs work

The css changes need to be applied to the D9 and D10 Version, but different changes for each branch! As described in #3! I can try to solve them and let @thomasfrobieter review afterwards.

Comment #7

Anybody

German

Porta Westfalica

CreditAttribution: Anybody at DROWL.de commented 31 March 2023 at 09:01

@Grevil yes please. You may create both MR's here. To make it easier for @thomas.frobieter to review.

Comment #8

Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commented 31 March 2023 at 09:05

Damn I should have read through the whole change record first...

To target all versions, simply combine the selectors

Comment #9

Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commented 31 March 2023 at 09:12

Assigned:

Grevil

» thomas.frobieter

And also the css files are minified, let's just fix this in one branch I am not 100% sure were the changes need to get applied, so I reassign @thomas.frobieter here.

Comment #10

Anybody

German

Porta Westfalica

CreditAttribution: Anybody at DROWL.de commented 31 March 2023 at 09:50

@Grevil: FYI: Minification will be removed on all Drupal project, still an open task for @thomas.frobieter when the time has come

Comment #11

18 April 2023 at 07:32

thomas.frobieter made their first commit to this issue’s fork.

Comment #12

thomas.frobieter

German

CreditAttribution: thomas.frobieter commented 18 April 2023 at 08:13

Status:

Needs work

» Fixed

Thx @Grevil! I agree, please merge this into 2.x and remove || ^10 and create a new 3.x branch with the proposed changes as separate issue afterwards. Assigned to @thomas.frobieter.

I disagree on this. #drupal-off-canvas is still present in D10, so there is no need to switch to the outer wrappers selector. So we should still have D9 compability, without extra release.

Important thing for the off-canvas styles to work: Uninstall GIN Layout Builder, as it contains a buggy sidebar. We should stick with the core styles, which seems more reliable now.

Comment #13

18 April 2023 at 08:13

thomas.frobieter committed 929a2c39 on 2.x authored by Grevil

Issue #3350661 by thomas.frobieter, Grevil, Anybody: Fix remaining...

Comment #14

thomas.frobieter

German

CreditAttribution: thomas.frobieter commented 18 April 2023 at 08:14

Status:

Fixed

» Closed (fixed)

Comment #15

Anybody

German

Porta Westfalica

CreditAttribution: Anybody at DROWL.de commented 18 April 2023 at 08:58

Status:

Closed (fixed)

» Fixed

@thomas.frobieter I'm fine with any decision, as it's your part, just want to ensure we don't do something wrong here due to this message from drupal upgrade status:

web/themes/custom/webksdct/css/modules/backend/layout_builder.min.css:
┌─────────┬──────┬─────────────────────────────────────────────────────┐
│ STATUS  │ LINE │                       MESSAGE                       │
├─────────┼──────┼─────────────────────────────────────────────────────┤
│ Manuell │ 0    │ The #drupal-off-canvas selector is deprecated in    │
│ überpr� │      │ drupal:9.5.0 and is removed from drupal:10.0.0. See │
│ �fen    │      │ https://www.drupal.org/node/3305664.                │
│         │      │                                                     │
└─────────┴──────┴─────────────────────────────────────────────────────┘

which says it will be removed?!

See https://www.drupal.org/node/3305664 where they say only the "-wrapper" one will be kept.

If you already read all that, fine! :)

Comment #16

thomas.frobieter

German

CreditAttribution: thomas.frobieter commented 18 April 2023 at 09:04

I've checked it in our D10 instance, its present. The way the new CSS reset of the offcanvas work is kinda forcing us to use the other id. In fact, it doesnt matter, as long as the selector specicicy is heigher then the reset - what is the case. Since we need to support D9 + D10, this is the way to go.

Otherwise we need to drop D9 support, I don't want to maintain two versions of this.

Comment #17

Anybody

German

Porta Westfalica

CreditAttribution: Anybody at DROWL.de commented 18 April 2023 at 11:36

D9 support may be dropped until end of this year. Until then it's easy to use cherry-picks to copy changes over.
You decide! :)

Comment #18

thomas.frobieter

German

CreditAttribution: thomas.frobieter commented 18 April 2023 at 11:59

Status:

Fixed

» Closed (fixed)

We have a working solution, so we should not make more work for ourselves than necessary.

Comment #19

Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commented 18 April 2023 at 14:08

@Anybody please read the change record!

https://www.drupal.org/node/3305664

To target all versions, simply combine the selectors. Be sure to test thoroughly.

#drupal-off-canvas:not(.drupal-off-canvas-reset),
#drupal-off-canvas-wrapper {
}

Both the d9 and d10 CSS selectors are present! So no harm, leaving the d9 selector.

Comment #20

thomas.frobieter

German

CreditAttribution: thomas.frobieter commented 18 April 2023 at 14:13

The mentioned solution from the change record will produce 200% CSS code, I don't see any need to do that.
We might switch the selectors when Drupal 9 is gone.

Howewer, we should not waste more time on this completely unimportant issue ;)

Comment #21

Anybody

German

Porta Westfalica

CreditAttribution: Anybody at DROWL.de commented 13 July 2023 at 08:42

@thomas.frobieter: This introduced a critical regression, please always review carefully.

The merge removed the Drupal 10 compatibility. Hopefully no other things were reverted / broken? Please take a look at the commit above again, if anything else is fine! I'm going to fix the D10 compatibility in the other issue now. See #3374318: Regression: Not Drupal 10 compatible anymore for the fix.

No idea how that happened... but a good reason for careful reviews.

Comment #22

Anybody

German

Porta Westfalica

CreditAttribution: Anybody at DROWL.de commented 13 July 2023 at 08:43

If anything else was broken by that commit, please create a separate issue for the fix.

Comment #23

thomas.frobieter

German

CreditAttribution: thomas.frobieter commented 13 July 2023 at 08:50

I've checked my commits, looks like there are only CSS and library.yml changes. So what exactly is the point here with the D10 compability? They check for stylesheets, really?!

So the message from https://www.drupal.org/project/drowl_admin/issues/3350661#comment-15014690 ist a blocker for D10 compability? This is so mad.

Comment #24

Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commented 13 July 2023 at 10:20

File	Size
2023-07-13 12_18_42-Issue #3350661_ Fix remaining Drupal 10 compatibility issues (!6) · Merge reques.png	17.85 KB

@thomas.frobieter, check them again:
screenshot

EDIT: Happens to the best! :)

Comment #25

Anybody

German

Porta Westfalica

CreditAttribution: Anybody at DROWL.de commented 13 July 2023 at 10:28

Re: #23 don't really get the comment. See #13 that's the code that was merged and should please be re-reviewed. It removed the Drupal 10 compatibility, as can be seen in #24 so we need to be sure nothing else was broken. As surely nobody removed the ^10 manually.

Do not commit anything here, please just review if all other parts of the commit were fine.