[META] Make Webform compatible with Drupal 10 [#3262067]

Problem/Motivation

Drupal 10 will be released soon #3251854: [META] Requirements for tagging Drupal 10.0.0-alpha1. We should investigate if webform is already compatible with it...

Known issues

The IE11 polyfill is interesting:

  1x: Support for IE Conditional Comments is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. See https://www.drupal.org/node/3102997
    1x in WebformLibrariesTest::testLibraries from Drupal\Tests\webform\Functional

According to https://www.drupal.org/node/3102997, that syntax does not work in IE11?

Not sure if D10, which also removes IE11 support will just silently ignore it or throw an error if set. ignoring it would be kinda nice assuming it actually does anything at all in IE11? Or you could just drop it, leaving that up to you.

Another fun one:

  1x: The module 'hal' is deprecated. See https://www.drupal.org/node/3223395#s-hal
    1x in WebformEntityReferenceItemNormalizerTest::testWebformEntityReferenceItemNormalization from Drupal\Tests\webform\Functional

leaving that for now. we can try if adding a composer.json dependency on drupal/hal will work as expected on all supported versions. in ERR, I also struggled with API changes in symfony 6, but you might not be affected by this because you just override a drupal specific method.

Update: It's now possible to add a require-dev dependency on drupal/hal. on 9.4+, that will download the contrib module and 9.3 and below will use the one in core.

And another problematic one:

1x: Calling Drupal\Core\Session\SessionManager::getId() outside of an actual existing session is deprecated in drupal:9.2.0 and will be removed in drupal:10.0.0. This is often used for anonymous users. See https://www.drupal.org/node/3006306
    1x in WebformNodeTest::testNode from Drupal\Tests\webform_node\Functional

Took me a moment to track this down. It's actually coming from \Drupal\webform\WebformSubmissionForm::addCacheableDependency, the session stuff. The stack trace is pure core, looks like adding that cache context when not having a session is problematic?

Symfony\Component\HttpFoundation\Session\Session->getId() (Line: 27)
Drupal\Core\Cache\Context\SessionCacheContext->getContext(NULL) (Line: 118)
Drupal\Core\Cache\Context\CacheContextsManager->convertTokensToKeys(Array) (Line: 317)
Drupal\Core\Render\RenderCache->createCacheID(Array) (Line: 93)
Drupal\Core\Render\RenderCache->set(Array, Array) (Line: 127)
Drupal\Core\Render\PlaceholderingRenderCache->set(Array, Array) (Line: 515)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 201)
Drupal\Core\Render\Renderer->render(Array, ) (Line: 241)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 564)

Comment	File	Size	Author
#32	3262067-32.patch	61.4 KB	jrockowitz
#31	3326360-6.patch	1.78 KB	jrockowitz
#22	Screen Shot 2022-11-22 at 11.44.02 PM.png	490.65 KB	kristen pol
#19	webform-d10-compatibility-3262067-19.patch	58.4 KB	berdir
#11	webform_extra_fixes.diff	1.73 KB	jitendrapurohit

Issue fork webform-3262067

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

3262067-deprecations changes, plain diff MR !205
Check out this branch for the first time

Check out existing branch, if you already have it locally
3262067-meta-make-webform changes, plain diff MR !139
Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

2 February 2022 at 17:20

paulocs created an issue. See original summary.

Comment #2

2 February 2022 at 17:27

paulocs opened merge request !139

Comment #3

paulocs

Portuguese, Brazil

Belo Horizonte

commented 2 February 2022 at 17:27

Lets check what errors will be triggered.

Comment #4

berdir

German

Switzerland

commented 12 July 2022 at 20:22

I've created an issue for jquery.once and made the issue for file_create_url() a child issue.

See also https://dev.acquia.com/drupal10/deprecation_status/projects/webform for a list of discovered issues, but I think that uses the wrong branch.

With that many require-dev dependencies, it will be extremely challenging to do a D10 test run, as many of them will likely not be compatible any time soon or possible ever if no longer maintained.

One option would be to temporarily remove all that are not yet compatible, then the tests requiring them will fail but we can still test all others.

I'd propose (also) doing what I did quite successfully in other projects, which is temporarily changing suppress-deprecations: true to false in drupalci.yml in this MR, then we can test on D9.

Comment #5

berdir

German

Switzerland

commented 12 July 2022 at 20:59

Just from a single test that I did run locally I got the following deprecation messages:

  3x: ModuleHandlerInterface::getImplementations() is deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. Instead you should use ModuleHandlerInterface::invokeAllWith() for hook invocations, or you should use ModuleHandlerInterface::hasImplementations() to determine if hooks implementations exist. See https://www.drupal.org/node/3000490
    3x in WebformElementManagedFilePreviewTest::testImageFileUpload from Drupal\Tests\webform\Functional\Element

  2x: Relying on entity queries to check access by default is deprecated in drupal:9.2.0 and an error will be thrown from drupal:10.0.0. Call \Drupal\Core\Entity\Query\QueryInterface::accessCheck() with TRUE or FALSE to specify whether access should be checked. See https://www.drupal.org/node/3201242
    2x in WebformElementManagedFilePreviewTest::testImageFileUpload from Drupal\Tests\webform\Functional\Element

  2x: The core/jquery.once asset library is 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
    2x in WebformElementManagedFilePreviewTest::testImageFileUpload from Drupal\Tests\webform\Functional\Element

  1x: Drupal\Core\File\MimeType\MimeTypeGuesser::guess() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use ::guessMimeType() instead. See https://www.drupal.org/node/3133341
    1x in WebformElementManagedFilePreviewTest::testImageFileUpload from Drupal\Tests\webform\Functional\Element

getImplementations() probably deserves a dedicated issue. This is a 9.4 deprecation and is usually dealt with a method_exists and fallback to avoid depending on that for now.

entity query as well. There are unsurprisingly a lot of them. A bit tricky to find them all but we can enable deprecations and grep for that in the output.

Comment #6

12 July 2022 at 21:07

Berdir opened merge request !205

Comment #7

berdir

German

Switzerland

commented 14 July 2022 at 20:46

core/jquery.form: That was added in #2853543: Scroll to top to the page when dialog has form validation errors. with the initial definition of the webform.ajax library. I'm pretty sure that was just copy pasted and is not actually required.

Comment #8

berdir

German

Switzerland

commented 19 July 2022 at 19:29

Issue summary:

View changes

Down to 45 failing tests. many are fixed with the latest patch in #3296068: [Drupal 9.4.x+] Test and one-off D10 deprecations, the remaining ones fall in 3 categories from what I see:

* remaining jquery once usages, that issue is WIP.
* integration tests with other contrib modules
* some tricky ones, documented in the issue summary.

Comment #9

berdir

German

Switzerland

commented 28 July 2022 at 20:26

Issue summary:

View changes

Down to 23 fails, one is a mistake with jquery.once that is now fixed.

Also an update on the hal situation. It's now possible to add a require-dev dependency on drupal/hal. on 9.4+, that will download the contrib module and 9.3 and below will use the one in core.

Comment #10

berdir

German

Switzerland

commented 29 July 2022 at 06:12

Tried to get a D10 test up and running without incompatible dev modules, but extending from base classes of those removed modules makes that impossible for now:

07:56:40 Fatal error: Uncaught Error: Class "Drupal\Tests\group\Functional\GroupBrowserTestBase" not found in /var/www/html/modules/contrib/webform/modules/webform_group/tests/src/Functional/WebformGroupBrowserTestBase.php:13

Comment #11

jitendrapurohit commented 7 August 2022 at 07:43

Status	File	Size
new	webform_extra_fixes.diff	1.73 KB

I wasn't able to get things working (create a webform, navigate to pages, etc) on drupal 10 without this extra patch -

Should we include this in the main MR for d10?

UPDATE - Sorry, i see i've created this patch against the wrong version. The 6.2.x already has this change included. Pls ignore the attached file.

Comment #12

berdir

German

Switzerland

commented 7 August 2022 at 08:14

The merge request here is just for testing purposes, all changes should go in specific issues, maybe the one off issue or a new one

Comment #13

kristen pol

she/her

English

Santa Cruz, CA, USA

commented 16 August 2022 at 20:41

@Berdir suggested the bot issue #3290604: Automated Drupal 10 compatibility fixes should be closed out so cross-linking.

Comment #14

jcnventura

English

commented 29 September 2022 at 15:14

Adding #3312637: [PHP 8.1] imagedestroy is deprecated as a related issue, in case support for Drupal 10 becomes a new major version, and that deprecated call can be removed from the code.

Comment #15

jrockowitz commented 29 September 2022 at 15:42

@jcventura Thank you for referencing those tickets.

I added this ticket as a parent to those tickets, which gives use a clean list of child tickets in the right sidebar.

Comment #16

18 October 2022 at 10:37

Rajeshreeputra made their first commit to this issue’s fork.

Comment #17

rajeshreeputra

he/him

Pune

commented 18 October 2022 at 11:06

Status:

Active

» Needs review

Comment #18

kristen pol

she/her

English

Santa Cruz, CA, USA

commented 20 October 2022 at 18:23

Issue tags:

+Drupal 10 compatibility

Tagging

Comment #19

berdir

German

Switzerland

commented 18 November 2022 at 07:31

Status	File	Size
new	webform-d10-compatibility-3262067-19.patch	58.4 KB

Rebased with latest from the one-off issue. Also adding a patch with the core compatibility updates.

Comment #20

berdir

German

Switzerland

commented 18 November 2022 at 15:55

Ok, all child issues were committed. What's left are the problems mentioned in the issue summary. Some are easy to fix, like adding a require-dev for drupal/hal.

Also looking at the remaining descriptions, there seem to be two assert calls with extra arguments that I missed. search for "more than one" on https://www.drupal.org/pift-ci-job/2524410.

Comment #21

kristen pol

she/her

English

Santa Cruz, CA, USA

commented 19 November 2022 at 01:13

Issue tags:

+Drupal 10 porting day

Tagging for visibility.

@Berdir Do you need help with any of those things?

Comment #22

kristen pol

she/her

English

Santa Cruz, CA, USA

commented 23 November 2022 at 07:46

Status	File	Size
new	Screen Shot 2022-11-22 at 11.44.02 PM.png	490.65 KB

Perhaps it is premature, but I needed to test a module that has webform as a dependency and everything worked as expected using the patch from #19. Thanks!

Comment #23

kristen pol

she/her

English

Santa Cruz, CA, USA

commented 23 November 2022 at 08:26

Actually, I did have a webform tooltip issue, but I don't know where it's coming from as I can't find a createTippy anywhere in the codebase. There is a createPopper in web/core/assets/vendor/shepherd/shepherd.min.js.map. I do see shepherd used in Claro so maybe the bug is there.

#3323311: Consultation webform phone field tooltip not working

Comment #24

jrockowitz commented 23 November 2022 at 14:01

I think supporting CKEditor5 is going to be a big challenge for D10.

@see #3322552: CKEditor 5 support

Comment #25

heddn

English

Illinois

commented 8 December 2022 at 22:16

See #3278740: D10 compatibility for when group.module will start supporting D10.

Comment #26

berdir

German

Switzerland

commented 9 December 2022 at 09:14

@jrockowitz: I wasn't able to catch up on the 40 (!) new comments in the ckeditor issue yet, I was wondering if you could commit this already sooner. i haven't seen any ckeditor tests and I think that integration is technically optional?

Some modules won't work yet if their dependencies aren't ready, but I assume the vast majority of users of webform aren't using specific integration modules for group, entity print and so on. One option would be to not mark them ready yet, but the modules themself area ready and it would IMHO be tedious to try and keep that in sync.

In the meantime, rebased the MR so it applies again against 6.2.x. Verified that https://git.drupalcode.org/project/webform/-/merge_requests/205.diff applies as a patch and works together with the lenient plugin.

Reminder to NOT use any merge request patches directly in real projects, as anyone can update that and literally inject code into your site.

Comment #27

berdir

German

Switzerland

commented 9 December 2022 at 09:21

If you commit/merge this, do _not_ commit the deprecation changes in drupalci.yml.

Comment #28

jrockowitz commented 9 December 2022 at 10:44

#3322552: CKEditor 5 support will need to be committed first, and then this patch.

I will also work to resolve some of the fixable broken tests to improve d10 support.

Reminder to NOT use any merge request patches directly in real projects, as anyone can update that and literally inject code into your site.

Is the current/only solution to still create and attach a patch to a ticket?

Comment #29

mark_fullmer

he/him

English

Tucson

commented 9 December 2022 at 17:55

Is the current/only solution to still create and attach a patch to a ticket?

As far as I know, yes. Our organization actually copies each patch we're using from drupal.org and then references those copies in our Composer patch declarations. If nothing else, this removes drupal.org servers as a dependency in our site build & deploy process.

Comment #30

jrockowitz commented 9 December 2022 at 18:58

I did a local review of all the failing tests, and I found two minor tweaks that were needed.

https://git.drupalcode.org/project/webform/-/commit/4786f7dd5d56cbdf593d...

Let's see if anyone has time to review the CKEditor 5 support. If not, I might still move ahead and hold off tagging another beta release fore a few days.

Comment #31

jrockowitz commented 11 December 2022 at 07:33

Status	File	Size
new	3326360-6.patch	1.78 KB

Re-rolling core_version_requirement patch.

Comment #32

jrockowitz commented 11 December 2022 at 07:39

Status	File	Size
new	3262067-32.patch	61.4 KB

Arg I'm focused on getting D10 support, but I'm also tired.

Comment #33

11 December 2022 at 15:06

0ed2659 committed on 6.2.x

Issue #3262067 by Berdir, paulocs, jrockowitz, Rajeshreeputra,...

Comment #34

jrockowitz commented 11 December 2022 at 15:07

Status:

Needs review

» Fixed

To push D10 support forward, I am committing the patch. I will create a new ticket to address the remaining broken tests, which I can use help with.

Comment #35

eelkeblok

he/him

Dutch

Netherlands 🇳🇱

commented 1 January 2023 at 13:50

Is there a plan issue that lists all remaining work to get Webform D10 ready? The [meta] in this issue's title made me think this is it, but this issue is fixed, while the last comment suggests there is more work to do (as does the core compatibility on the last 6.2 beta release).