Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JeroenT created an issue. See original summary.

JeroenT’s picture

Status: Active » Needs review
FileSize
413 bytes
JeroenT’s picture

I tried this module with Drupal 9 and webform 6.0 and the code is working. Also, locally the tests are passing after applying patch #3. So I guess this module is ready for a new Drupal 9 compatible release.

On drupal.org the tests for Drupal 9 are failing because the testbot tries to run the tests with Webform 5. I'm not sure if we can fix that right now.

I'm getting some deprecation errors for Drupal 10 after running the tests, but I created 2 separate issues for that:

stefanos.petrakis@gmail.com’s picture

Status: Needs review » Needs work

After talking some about this with Jeroen, and also looking at the bot's logs, I think the fix for having the tests running for D9 would be to require a newer version than the one the bot uses (cached?). The failing tests from #3 were using a 5.13 version of Webform that threw a ServiceNotFoundException.

Will commit a fix for that and let the tests run again to double-check that.

stefanos.petrakis@gmail.com’s picture

Related issues: +#3128885: Drupal 9 readiness

So, here is the problem (the bot's problem really, but green tests on d.o. are a sign of quality and assurance I believe).

If we run a D8 test against our code, the Webform dependency resolves to 5.x matches and all work fine.
If we run a D9 test against our code, the Webform dependency still resolves to 5.x which should fail since 5.x is not D9 compatible (per Webform). Ideally, the D9 test suite could enforce checking against the dependencies version restrictions for Webform and once 5.x failed (since it requires D8), switch to the alternative (see "drupal/webform": "^5.0 || ^6.0") which would be a 6.x version.

I have no better suggestion at the moment apart from the following:

8.1.x will stay as is so that it can be tested on d.o. upon further development.
A new 2.x version will be spawned from the current one and become the main development branch. Related issues should be backported to 8.y.x. if needed. The 2.x version will require both D9 and Webform 6.x in order to have d.o. testing failing sensibly.

Is this making sense to you Jeroen?

P.S.: My previous attempts today were more in the line of trying out the >= operator inside the info.yml file.

Mixologic’s picture

The issue here is that packages.drupal.org is getting confused by your 'test-dependencies' declaration in your info.yml.

https://git.drupalcode.org/project/webform_translation_permissions/-/blo...

Since webform is already a dependency, you should remove that line. test-dependencies are really just translated into require-dev, and if you already have a composer.json, probably should not be there at all, and only use the require-dev in composer.json.

The composer facade does not allow for unbounded constraints like > or >= because in the vast majority of the cases, that will break your dependency chain when a new major api comes out that is incompatible. As soon as webform 7 comes out, that may or may not be invalid.
As such, it changes that to ~5.22 because its compensating for the fact that there is a lot of drupal 7 code out there that isnt semver specific, and so >, or >= used to make more sense prior to semantic versioning/packages.drupal.org.

What you have in composer.json is the right thing to do: https://git.drupalcode.org/project/webform_translation_permissions/-/blo...

stefanos.petrakis@gmail.com’s picture

Status: Needs work » Fixed

So, this is finally resolved with the help of mixologic.
The workaround to allow both D8 and D9 testing by the bot was to skip webform 5.1 till 5.14 for D8.
The reason was that 5.9 till 5.14-beta2 were declaring D9 compatibility (with deprecated code), which was causing D9 tests to fail.
After the changes, the D9 testing correctly uses 6.x.* as a dependency.

Thanks for the patience and the help.

  • stefanos.petrakis committed bf5d4a1 on 8.x-1.x
    Issue #3156749 by JeroenT, stefanos.petrakis, Mixologic: Create a new...

Status: Fixed » Closed (fixed)

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