Problem/Motivation

Running a PHPUnit test with webform at the latest core dev 11.x, we get a logic error and the test halts with the following: 

LogicException: The hook update_dependencies on class Drupal\webform\Hook\WebformInstallUpdateHooks does not support attributes and must remain procedural.

Here is an example phpunit job from Gitlab Templates Downstream, the project used to test changes to the the contrib Gitlab Templates. The error is caused only by requiring webform in the project's composer.json. It is not actually being enabled or used in the tests.

This is not an urgent problem, as Core 11.3.0 is not planned for release until December 2025, according to the current release cycle, but I am raising this now, at the point when I found it, just so that it is recorded.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork webform-3547627

Command icon Show commands

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

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

About issue forks

Comments

jonathan1055 created an issue. See original summary.

jonathan1055 opened merge request !742

jonathan1055’s picture

jonathan1055 commented

I have created MR742 to demonstrate the problem by running against 11.3-dev, using OPT_IN_TEST_NEXT_MINOR: 1. The PHPUnit Next Minor job fails with the same error.

jrockowitz’s picture

jrockowitz commented
Status: Active » Needs work

Both modules have D11 releases. I think you can remove the _LENIENT_ALLOW_LIST

https://www.drupal.org/project/select2
https://www.drupal.org/project/styleguide

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes

Both modules have D11 releases. I think you can remove the _LENIENT_ALLOW_LIST

Yes they do. It's strange because the first pipeline (without _LENIENT_ALLOW_LIST) failed with unresolved requirements:
https://git.drupalcode.org/issue/webform-3547627/-/jobs/6599563#L76

  Problem 1
    - Root composer.json requires drupal/core-recommended 11.x-dev -> satisfiable by drupal/core-recommended[11.x-dev].
    - Root composer.json requires drupal/select2 1.x-dev -> satisfiable by drupal/select2[1.x-dev (alias of dev-1.x)].
    - drupal/core[9.0.0-alpha1, ..., 9.0.0-alpha2] require symfony/psr-http-message-bridge ^1.2.0 -> satisfiable by symfony/psr-http-message-bridge[v1.2.0, v1.3.0].
    - drupal/core[9.0.10, ..., 9.0.x-dev] require php ^7.3 -> your php version (8.3.25) does not satisfy that requirement.
    - drupal/core-recommended 11.x-dev requires drupal/core 11.x-dev -> satisfiable by drupal/core[11.x-dev].
    - drupal/select2 dev-1.x requires drupal/core ^9 || ^10 -> satisfiable by drupal/core[9.0.0-alpha1, ..., 9.5.x-dev, 10.0.0-alpha1, ..., 10.6.x-dev].
    - symfony/psr-http-message-bridge[v1.2.0, ..., v1.3.0] require php ^7.1 -> your php version (8.3.25) does not satisfy that requirement.
    - You can only install one version of a package, so only one of these can be installed: drupal/core[8.0.0-beta6, ..., 8.9.x-dev, 9.0.0-alpha1, ..., 9.5.x-dev, 10.0.0-alpha1, ..., 10.6.x-dev, 11.0.0-alpha1, ..., 11.x-dev].
    - You can only install one version of a package, so only one of these can be installed: drupal/core[8.7.0-alpha1, ..., 8.9.x-dev, 9.0.0-alpha1, ..., 9.5.x-dev, 10.0.0-alpha1, ..., 10.6.x-dev, 11.0.0-alpha1, ..., 11.x-dev].
    - drupal/select2 1.x-dev is an alias of drupal/select2 dev-1.x and thus requires it to be installed too.

Reading the full list, it seems the bit that caused the problem was: 

    - drupal/select2 dev-1.x requires drupal/core ^9 || ^10 -> satisfiable by drupal/core[9.0.0-alpha1, ..., 9.5.x-dev, 10.0.0-alpha1, ..., 10.6.x-dev].

we only see ^9 || ^10 there is no ^11 which is surprising.

demeritcowboy’s picture

demeritcowboy commented

We're seeing this too but from actually trying to enable the module during a test, not from composer. This is with 11.x-dev core (953cd79) and 6.x-dev webform (2bbf12a).

LogicException: The hook update_dependencies on class Drupal\webform\Hook\WebformInstallUpdateHooks does not support attributes and must remain procedural.

/home/runner/drupal/web/core/lib/Drupal/Core/Hook/HookCollectorPass.php:588
/home/runner/drupal/web/core/lib/Drupal/Core/Hook/HookCollectorPass.php:426
/home/runner/drupal/web/core/lib/Drupal/Core/Hook/HookCollectorPass.php:375
/home/runner/drupal/web/core/lib/Drupal/Core/Hook/HookCollectorPass.php:137
/home/runner/drupal/vendor/symfony/dependency-injection/Compiler/Compiler.php:73
/home/runner/drupal/vendor/symfony/dependency-injection/ContainerBuilder.php:814
/home/runner/drupal/web/core/lib/Drupal/Core/DrupalKernel.php:1397
/home/runner/drupal/web/core/lib/Drupal/Core/DrupalKernel.php:916
/home/runner/drupal/web/core/lib/Drupal/Core/DrupalKernel.php:829
/home/runner/drupal/web/core/lib/Drupal/Core/Extension/ModuleInstaller.php:729
/home/runner/drupal/web/core/lib/Drupal/Core/Extension/ModuleInstaller.php:320
/home/runner/drupal/web/core/lib/Drupal/Core/Extension/ModuleInstaller.php:229
/home/runner/drupal/web/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/home/runner/drupal/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:511
/home/runner/drupal/web/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php:83
/home/runner/drupal/web/core/tests/Drupal/Tests/BrowserTestBase.php:537
/home/runner/drupal/web/core/tests/Drupal/Tests/BrowserTestBase.php:343
/home/runner/work/webform_civicrm/webform_civicrm/tests/src/FunctionalJavascript/CiviCrmTestBase.php:25

CiviCrmTestBase.php:25 refers to a call to parent::setUp() in a mink test, and from the stack trace appears to be installing modules: https://github.com/colemanw/webform_civicrm/blob/90fbd8558ac3425151f754d....

Is the attribute declared here needed for something? https://git.drupalcode.org/project/webform/-/blob/2bbf12a24ac21ead7a0257...

jonathan1055’s picture

jonathan1055 commented

Thanks @demeritcowboy, that is useful info.

The attributes were added back in April on #3487957: Discuss process for converting webform to OOP Hooks. That issue is long and complex, and MR598 had modifications to 238 files. Many of the changes are correct, I am sure, but maybe this one was not? The pipelines back then were run with the latest 'current' core, which was 11.1. The problem seems to have been introduced in Core changes for 11.3 as the tests pass at 11.2 at the moment. That's why I have add the opt-in for "next minor" in MR742 in this issue.

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review

I have moved webform_update_dependencies() back to into includes/webform.install.update.inc so that it is procedural again in this commit, undoing one little part of the changes from on #3487957: Discuss process for converting webform to OOP Hooks. This has removed all of errors of the form:

LogicException: The hook update_dependencies on class Drupal\webform\Hook\WebformInstallUpdateHooks does not support attributes and must remain procedural.

in the 'next minor' phpunit job and the log size has reduced from 14k lines to 10k. There are still other errors, but they were also present in the initial pipeline phpunit before any changes. So it's not fixed at 11.3 but is better than before.

@demeritcowboy can you re-try using the changes in this MR?

demeritcowboy’s picture

demeritcowboy commented

Thanks I had tried a variation of that and I just tried yours now and in both cases yes it gets rid of the logic error but it crashes phpunit with no stacktrace and just the message Test was run in child process and ended unexpectedly. That may or may not be related - my next step is to try a local install and see if this comes up.

demeritcowboy’s picture

demeritcowboy commented

In the UI the problem is easy to reproduce, just install drupal 11.x-dev and then webform 6.3.x-dev and try to install webform.

The patch fixes the logic problem but it still gives a fatal error until I also make this patch, which seems unrelated but is a blocker to using the module:

diff --git a/src/EntityStorage/WebformEntityStorageTrait.php b/src/EntityStorage/WebformEntityStorageTrait.php
index 77bf3f8fd..87608d4fc 100644
--- a/src/EntityStorage/WebformEntityStorageTrait.php
+++ b/src/EntityStorage/WebformEntityStorageTrait.php
@@ -82,7 +82,7 @@ trait WebformEntityStorageTrait {
    * @return \Drupal\Core\Entity\EntityStorageInterface
    *   Entity storage or NULL.
    */
-  public function __get($name) {
+  public function __get($name): mixed {
     if (isset($this->entityStorageToTypeMappings[$name])) {
       $entity_type = $this->entityStorageToTypeMappings[$name];
       $class_name = get_class($this);
jonathan1055’s picture

jonathan1055 commented

That's great news, thanks for the info. Independently, and before I saw your result in #12, I wondered if running the pipeline phpunit tests with _PHPUNIT_CONCURRENT: 0 to use the plain phpunit binary might give any different information. The tests have been using _PHPUNIT_CONCURRENT: 1 which runs the core run-test.sh script. And yes it confirms what you discovered - we get hundreds of the error

PHPUnit\Framework\Exception: PHP Fatal error:
Declaration of Drupal\webform\EntityStorage\WebformEntityStorageTrait::__get($name) must be compatible with Drupal\Core\Entity\ContentEntityStorageBase::__get(string $name): mixed in /builds/issue/webform-3547627/src/EntityStorage/WebformEntityStorageTrait.php on line 85

See https://git.drupalcode.org/issue/webform-3547627/-/jobs/6614518#L685

I will make the change you suggest and see what we get.

jonathan1055’s picture

jonathan1055 commented

I have skipped some jobs, and selected just a few tests to run, to get quicker results. Interestingly, with the code as-is, we see the phpstan warning:

------ ---------------------------------------------------------------- 
  Line   src/EntityStorage/WebformEntityStorageTrait.php (in context of class  
         Drupal\webform\WebformSubmissionStorage)                              
 ------ ---------------------------------------------------------------- 
  85     Return type mixed of method                                           
         Drupal\webform\WebformSubmissionStorage::__get() is not covariant     
         with return type mixed of method                                      
         Drupal\Core\Entity\ContentEntityStorageBase::__get().                 
 ------ ----------------------------------------------------------------

https://git.drupalcode.org/issue/webform-3547627/-/jobs/6619707#L102
This looks like it is related to the change you suggest.

jonathan1055’s picture

jonathan1055 commented

That has fixed the small sample of failing tests
https://git.drupalcode.org/issue/webform-3547627/-/jobs/6620075
There are still lots of deprecations, but this is a good step forward.

But interestingly, restoring _PHPUNIT_CONCURRENT: 1 to use run-tests.sh we get failures. Either there is a problem in the webform code, or paragraphs or run-tests.sh
https://git.drupalcode.org/issue/webform-3547627/-/jobs/6620132#L61

jonathan1055’s picture

jonathan1055 commented
Status: Needs review » Needs work

Setting back to Needs Work, while I investigate the missing class error

PHP Fatal error:  Uncaught Error: Class "Drupal\Tests\feeds\Kernel\FeedsKernelTestBase" not found
in /builds/issue/webform-3547627/web/modules/contrib/paragraphs/tests/src/Kernel/Feeds/Target/ParagraphsTest.php:14
jonathan1055’s picture

jonathan1055 commented
Related issues: +#3548261: run-tests.sh can fail during testDiscovery on 3rd-party modules

I have raised an issue in Gitlab Templates, and will work on that too.

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review

I have removed the temporary changes and this is ready for review. In addition to fixing the update_dependencies hook, this MR also sets OPT_IN_TEST_NEXT_MINOR: 1 so that Webform will now always check against the upcoming minor version. I have also marked three tests with @group webform_fail_at_11_3 which will allow quick and easy re-running of that group, when working on the fixes for Core 11.3

demeritcowboy’s picture

demeritcowboy commented

I'm not familiar enough with gitlab CI to comment on those parts but we are using the changes in these files and it's working for us:

  • webform.install.update.inc
  • WebformEntityStorageTrait.php
  • WebformInstallUpdateHooks.php
  • webform.services.yml
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Priority: Normal » Major

Should we split out the actual fix from the gitlab CI changes as this is definitely wrong and essentially just a revert to how it was?

jonathan1055’s picture

jonathan1055 commented

this is definitely wrong and essentially just a revert to how it was

Just checking, when you say "this is wrong" do you mean the current commited code is wrong? or that the fix suggested here is wrong? I think you mean the former, but just want to make sure.

Yes I could move the Gitlab CI changes to a separate MR in this issue or into separate issue. But the error in webform is only evident when testing against 11.3-dev, (aka 11.x or "next minor"). So the gitlab CI change to opt in to 'next minor' is needed to demonstrate the fix. The other CI change (moving _LENIENT_ALLOW_LIST to the top-level global variable) is not just convenient for this change, and is needed for the job to run at 11.x, but it also allows easier running of a pipeline via the UI. If you try a UI pipeline and do not specify _LENIENT_ALLOW_LIST = select2, styleguide then all the composer jobs fail, because the blank value from the form takes precdence over the job-level coded value in the .gitlab-ci.yml. I could make a change here to repeat the job-level chnage for 'next minor' but that is still not the best way, which is to set the value at the global level.

The three tests that I have marked with @group @group webform_fail_at_11_3 I will move to a separate issue, as that is unrelated.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes, I meant converting webform_update_dependencies() to WebformInstallUpdateHooks.php is wrong and it's correct to revert this.

Not a maintainer, I'll leave the decision on what to include in this issue with the maintainers, just think it *might* be easier to get it committed without that. I don't know if tests against D11 ever worked, but the select2 change for example is easily something that could result in discussions, select2 has a new major version that is D11 compatible, so might want to allow that instead, but that might require other changes...

jonathan1055’s picture

jonathan1055 commented

Thanks for clarifying.

I've opened #3550117: Make tests pass on "next minor", Drupal 11.4 and removed those changes from here.

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

Looks right to me, thanks!

Yeah I only just found that hook is procedural only recently the merge to exclude it from conversion hasn't been merged yet.

jrockowitz’s picture

jrockowitz commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

jonathan1055’s picture

jonathan1055 commented

Now that this is merged it has unblocked #3550117: Make tests pass on "next minor", Drupal 11.4

Status: Fixed » Closed (fixed)

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