Problem/Motivation

WebformBreadcrumbBuilderTest uses @dataProvider methods. These must be static in PHPUnit 10 and newer.

This is currently causing a test failure and a phpstan error.

Proposed resolution

Make these methods static.

Remaining tasks

Implement.

User interface changes

None.

API changes

None.

Data model changes

None.

Issue fork webform-3538392

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

liam morland created an issue. See original summary.

divyansh.gupta’s picture

divyansh.gupta commented
Assigned: Unassigned » divyansh.gupta

Working on it!!

divyansh.gupta opened merge request !666

divyansh.gupta’s picture

divyansh.gupta commented
Assigned: divyansh.gupta » Unassigned
Status: Active » Needs review

Made the changes
Please review!!

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Needs review » Needs work

The methods that need to be changed are not the ones that have a @dataProvider annotation. It is the methods named in the @dataProvider annotation. See the phpstan reports for the current 6.3.x branch. Also, making a method static requires removing all use of $this inside the method.

cilefen’s picture

cilefen commented

@divyansh.gupta: Just to clarify further: It is not only removing usages of $this, but refactoring to maintain the same functionality.

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

idebr’s picture

idebr commented
Title: Make @dataProvider methods static » Make WebformBreadcrumbBuilderTest @dataProvider methods static
Status: Needs work » Needs review

The WebformBreadcrumbBuilderTest @dataProvider is now a static method

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

There are still several test failures in the latest test-run, will those all be fixed in this issue or in several other issues?
I would suggest doing it all in this issue, since that would reduce the number of needed rebases for the other patches in the queue

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

It would be great to be everything passing and I'm OK with doing it all in this issue.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

Can someone explain the adding of #[LegacyModuleImplementsAlter]? Is this completing work from another issue?

  • liam morland committed 3a4a642b on 6.3.x 
    Issue #3538392: Fix test failure in WebformElementCaptchaTest

  • liam morland committed 3a4a642b on 6.x 
    Issue #3538392: Fix test failure in WebformElementCaptchaTest

  • liam morland committed b96d6a97 on 6.2.x 
    Issue #3538392: Fix test failure in WebformElementCaptchaTest
liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Title: Make WebformBreadcrumbBuilderTest @dataProvider methods static » Make tests pass on 6.3.x

Test are now passing on 6.3.x on Drupal 10. Now let's get them to pass on Drupal 11.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Needs review » Needs work
jrockowitz’s picture

jrockowitz commented

Below are the remaining broken tests. I am going to take a crack at this now.

[0m[31m  41.069s Drupal\Tests\webform\Functional\Element\WebformElementOtherTest           1 passed, 1 failed, 1 log(s)
[0m[31m  27.655s …_export_import\Functional\WebformSubmissionImportExportFunctionalTest    1 passed, 1 failed, 1 log(s)
[0m[31m  10.591s Drupal\Tests\webform\Functional\Cache\WebformCacheTest                    0 passed, 1 failed, 1 log(s)
[0m[31m  10.084s Drupal\Tests\webform\Functional\Composite\WebformCompositeTest            0 passed, 1 failed, 1 log(s)
[0m[31m   9.256s Drupal\Tests\webform\Functional\Element\WebformElementCodeMirrorTest      0 passed, 1 failed, 1 log(s)
[0m[31m   9.527s Drupal\Tests\webform\Functional\Element\WebformElementCounterTest         0 passed, 1 failed, 1 log(s)
[0m[31m   9.699s Drupal\Tests\webform\Functional\Element\WebformElementDateTimeTest        0 passed, 1 failed, 1 log(s)
[0m[31m  12.097s Drupal\Tests\webform\Functional\Element\WebformElementHtmlEditorTest      0 passed, 1 failed, 1 log(s)
[0m[31m  12.512s Drupal\Tests\webform\Functional\Element\WebformElementMediaFileTest       0 passed, 1 failed, 1 log(s)
[0m[31m  11.656s Drupal\Tests\webform\Functional\Element\WebformElementMessageTest         0 passed, 1 failed, 1 log(s)
[0m[31m  12.405s Drupal\Tests\webform\Functional\Element\WebformElementRatingTest          0 passed, 1 failed, 1 log(s)
[0m[31m  11.934s Drupal\Tests\webform\Functional\Element\WebformElementReadonlyTest        0 passed, 1 failed, 1 log(s)
[0m[31m  11.695s Drupal\Tests\webform\Functional\Element\WebformElementSectionTest         0 passed, 1 failed, 1 log(s)
[0m[31m  11.139s Drupal\Tests\webform\Functional\Element\WebformElementStatesTest          0 passed, 1 failed, 1 log(s)
[0m[31m  10.458s …pal\Tests\webform\Functional\Element\WebformElementTermsOfServiceTest    0 passed, 1 failed, 1 log(s)
[0m[31m   9.132s …l\Tests\webform\Functional\Element\WebformElementValidateRequiredTest    0 passed, 1 failed, 1 log(s)
[0m[31m  14.917s Drupal\Tests\webform\Functional\Paragraphs\WebformParagraphsTest          0 passed, 1 failed, 1 log(s)
[0m[31m  43.452s Drupal\Tests\webform\Functional\States\WebformStatesServerTest            0 passed, 1 failed, 1 log(s)
[0m[31m  10.361s …ple_element_properties\Functional\WebformExampleElementPropertiesTest    0 passed, 1 failed, 1 log(s)
[0m[31m  10.030s …l\Tests\webform_image_select\Functional\WebformImageSelectElementTest    0 passed, 1 failed, 1 log(s)
[0m[31m  17.081s …ests\webform_entity_print\Functional\WebformEntityPrintFunctionalTest    0 passed, 1 failed, 1 log(s)
[0m[31m   6.607s Drupal\Tests\webform_node\Functional\WebformNodeTranslationTest           0 passed, 1 errored, 1 log(s)
[0m[31m  10.281s Drupal\Tests\webform_schema\Functional\WebformSchemaTest                  0 passed, 1 errored, 1 log(s)
[0m[31m  17.615s …al\Tests\webform_scheduled_email\Functional\WebformScheduledEmailTest    0 passed, 1 failed, 1 log(s)
jrockowitz’s picture

jrockowitz commented

A bunch of the tests are broken due to changes in the rendered output, and the few remaining broken tests are a bit trickier.

Fixed

  • Drupal\Tests\webform\Functional\Element\WebformElementOtherTest - Removed aria-required="true" attribute
  • Drupal\Tests\webform\Functional\Element\WebformElementCodeMirrorTest - Added resize-vertical to textarea
  • Drupal\Tests\webform\Functional\Element\WebformElementCounterTest - Added resize-vertical to textarea
  • Drupal\Tests\webform\Functional\Element\WebformElementHtmlEditorTest - Added resize-vertical to textarea
  • Drupal\Tests\webform\Functional\Element\WebformElementMediaFileTest - Changed image_file_jpg_modal.jpg.webp to image_file_jpg_modal.jpg.avif
  • Drupal\Tests\webform\Functional\Element\WebformElementMessageTest - Change attribute order
  • Drupal\Tests\webform\Functional\Element\WebformElementRatingTest- Removed aria-required="true" attribute
  • Drupal\Tests\webform\Functional\Element\WebformElementReadonlyTest - Added resize-vertical to textarea
  • Drupal\Tests\webform\Functional\Element\WebformElementSectionTest - Removed aria-required="true" attribute
  • Drupal\Tests\webform\Functional\Element\WebformElementStatesTest- Added resize-vertical to textarea
  • Drupal\Tests\webform\Functional\Element\WebformElementTermsOfServiceTest - Removed aria-required="true" attribute
  • Drupal\Tests\webform\Functional\Element\WebformElementValidateRequiredTest - Removed aria-required="true" attribute
  • Drupal\Tests\webform_submission_export_import\Functional\WebformSubmissionImportExportFunctionalTest - Ignore the changed property.
  • Drupal\Tests\webform\Functional\Composite\WebformCompositeTest - Removed aria-required="true" attribute
  • Drupal\Tests\webform_example_element_properties\Functional\WebformExampleElementPropertiesTest - Removed aria-required="true" attribute
  • Drupal\Tests\webform_image_select\Functional\WebformImageSelectElementTest - Removed aria-required="true" attribute
  • Drupal\Tests\webform_schema\Functional\WebformSchemaTest - Adjust rendering
  • Drupal\Tests\webform\Functional\States\WebformStatesServerTest - Removed aria-required="true" attribute
  • Drupal\Tests\webform\Functional\Paragraphs\WebformParagraphsTest - Change entity comparison to uuid comparison
  • Drupal\Tests\webform_entity_print\Functional\WebformEntityPrintFunctionalTest - Attribute order changed

TDB

  • Drupal\Tests\webform\Functional\Element\WebformElementDateTimeTest - Still have validation issues
  • Drupal\Tests\webform\Functional\Cache\WebformCacheTest - Needs work
  • Drupal\Tests\webform_scheduled_email\Functional\WebformScheduledEmailTest - Needs work
  • Drupal\Tests\webform_node\Functional\WebformNodeTranslationTest - webform_node_test_translation.module won't install
jrockowitz’s picture

jrockowitz commented

Fixed

  • Drupal\Tests\webform\Functional\Element\WebformElementDateTimeTest - Set date format to 'none' if the date element is set to 'none'.
  • Drupal\Tests\webform\Functional\Cache\WebformCacheTest - Needed to recreate webform submission
  • Drupal\Tests\webform_node\Functional\WebformNodeTranslationTest - Needed container_rebuild_required: true in *.info.

Needs work

  • Drupal\Tests\webform_scheduled_email\Functional\WebformScheduledEmailTest
jrockowitz’s picture

jrockowitz commented

Fixed

  • Drupal\Tests\webform_scheduled_email\Functional\WebformScheduledEmailTest - Save $webform_schedule
jrockowitz’s picture

jrockowitz commented

Hmm... I am seeing random test failures with the below errors, which I think is more of a CI/CD issue.

[4mWebform Element Ignored Properties (Drupal\Tests\webform\Functional\Element\WebformElementIgnoredProperties)[0m
    [33m ✘ [0mIgnored properties
       [33m┐[0m
       [33m├[0m [43;30mUnexpectedValueException: RecursiveDirectoryIterator::__construct(/builds/project/webform/web/sites/simpletest/45046828/files/php/twig/68a3d5231cd3b_form-element.html.twig_GxylLNVWITg5aAeazUkhb70Op): Failed to open directory: No such file or directory[0m
       [33m│[0m
       [33m│[0m [2m/[22mbuilds[2m/[22mproject[2m/[22mwebform[2m/[22mweb[2m/[22mcore[2m/[22mlib[2m/[22mDrupal[2m/[22mCore[2m/[22mHook[2m/[22mHookCollectorPass.php[2m:[22m[34m513[0m
       [33m│[0m [2m/builds/project/webform/web/core/lib/Drupal/Core/Hook/HookCollectorPass.php[22m[2m:[22m[34m463[0m
       [33m│[0m [2m/builds/project/webform/web/core/lib/Drupal/Core/Hook/HookCollectorPass.php[22m[2m:[22m[34m375[0m
       [33m│[0m [2m/builds/project/webform/web/core/lib/Drupal/Core/Hook/HookCollectorPass.php[22m[2m:[22m[34m137[0m
       [33m│[0m [2m/builds/project/webform/[22mvendor[2m/[22msymfony[2m/[22mdependency-injection[2m/[22mCompiler[2m/[22mCompiler.php[2m:[22m[34m73[0m
       [33m│[0m [2m/builds/project/webform/vendor/symfony/dependency-injection/[22mContainerBuilder.php[2m:[22m[34m814[0m
       [33m│[0m [2m/builds/project/webform/[22mweb[2m/[22mcore[2m/[22mlib[2m/[22mDrupal[2m/[22mCore[2m/[22mDrupalKernel.php[2m:[22m[34m1399[0m
       [33m│[0m [2m/builds/project/webform/web/core/lib/Drupal/Core/DrupalKernel.php[22m[2m:[22m[34m915[0m
       [33m│[0m [2m/builds/project/webform/web/core/lib/Drupal/Core/DrupalKernel.php[22m[2m:[22m[34m828[0m
       [33m│[0m [2m/builds/project/webform/web/core/lib/Drupal/Core/[22mExtension[2m/[22mModuleInstaller.php[2m:[22m[34m729[0m
       [33m│[0m [2m/builds/project/webform/web/core/lib/Drupal/Core/Extension/ModuleInstaller.php[22m[2m:[22m[34m320[0m
       [33m│[0m [2m/builds/project/webform/web/core/lib/Drupal/Core/Extension/ModuleInstaller.php[22m[2m:[22m[34m229[0m
       [33m│[0m [2m/builds/project/webform/web/core/lib/Drupal/Core/[22mProxyClass[2m/[22mExtension[2m/[22mModuleInstaller.php[2m:[22m[34m83[0m
       [33m│[0m [2m/builds/project/webform/web/core/lib/Drupal/Core/[22mTest[2m/[22mFunctionalTestSetupTrait.php[2m:[22m[34m511[0m
       [33m│[0m [2m/builds/project/webform/web/core/[22mtests[2m/Drupal/[22mTests[2m/[22mBrowserTestBase.php[2m:[22m[34m537[0m
       [33m│[0m [2m/builds/project/webform/web/core/tests/Drupal/Tests/BrowserTestBase.php[22m[2m:[22m[34m343[0m
       [33m│[0m [2m/builds/project/
jrockowitz’s picture

jrockowitz commented
Status: Needs work » Needs review

Woot! All the tests are passing without any major code changes.

https://git.drupalcode.org/project/webform/-/pipelines/576493

I think we should merge this and move on to preparing for stable D11 release.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

Fantastic!

At lot the changes could avoid having a different test in Drupal 10 and 11 if they used XPath. That allows the test to test for the needed elements and attributes without checking for the exact markup.

Is adding LegacyModuleImplementsAlter related to #3487957: Discuss process for converting webform to OOP Hooks? Perhaps it should have its own commit attached to that issue. That change also introduces a phpstan error. It should have an exception added.

jrockowitz’s picture

jrockowitz commented

At lot the changes could avoid having a different test in Drupal 10 and 11 if they used XPath. That allows the test to test for the needed elements and attributes without checking for the exact markup.

I am open to any MR that converts HTML checks to XPath.

Is adding LegacyModuleImplementsAlter related to #3487957: Discuss process for converting webform to OOP Hooks? Perhaps it should have its own commit attached to that issue. That change also introduces a phpstan error. It should have an exception added.

I added use Drupal\Core\Hook\Attribute\LegacyModuleImplementsAlter; to webform.module. Let's see if PHPStan is now okay.

jrockowitz’s picture

jrockowitz commented

I had no idea the conversion to XPath was that simple.

In PHPStorm, I was able to prompt AI to "Convert to `responseContains` to `elementExists` with XPath" for a single assertion and it worked, but I don't have AI support setup for the whole webform code base yet.

@lkmorlan Do as much work as you feel comfortable and if the tests are passing you can merge this at anytime.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

I did one XPath conversion manually. Even manually is pretty simple. I hadn't thought it trying a LLM on it.

phpstan is happy with LegacyModuleImplementsAlter now (though not on Drupal 10). Is that addition a follow-up to #3487957: Discuss process for converting webform to OOP Hooks? If so, I'll can move that into a separate commit.

jrockowitz’s picture

jrockowitz commented

I did not add the `LegacyModuleImplementsAlter`, but it seems to be okay. Feel free to move it to a separate commit.

jrockowitz’s picture

jrockowitz commented

@liam, could we please merge this sooner rather than later, because we will need to update a lot of existing MRs to ensure all the tests are passing?

  • liam morland committed 82b579b7 on 6.3.x 
    Issue #3538392: Use XPath in WebformElementValidateRequiredTest

  • liam morland committed f9ed1dd2 on 6.3.x 
    Issue #3538392: Update tests to handle differences in Drupal 11...

  • liam morland committed 82b579b7 on 6.x 
    Issue #3538392: Use XPath in WebformElementValidateRequiredTest

  • liam morland committed f9ed1dd2 on 6.x 
    Issue #3538392: Update tests to handle differences in Drupal 11...

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Needs review » Fixed

Tests are now passing on 6.3.x. Thanks all!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

I'm not super familiar with the new contribution records yet, but it seems like no one has gotten credit for this issue yet?

jrockowitz’s picture

jrockowitz commented

@borisson_ Thank you for the nudge to assign credits

Status: Fixed » Closed (fixed)

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