Remove webform as a dependency

Hi Villette,

Thanks for your patch and suggestions.

The issue that I had with D8, was that as a submodule, I couldn't find a way specify the dependency only for that - the dependency only seemed to work in the base module.

If we leave it out as a dependency, then I think users may get confused why the example sub-module doesn't work.

I think the solution may be to use webform 6 in composer. We cannot specify versions in the info file.

There shouldn't be any issues, unless the Webform API has changed dramatically.

Sorry it took me so long to get back. End of year was a crazy workload and I didn't even have time to breath. Looking at https://www.drupal.org/project/webform, the minimum core version for v6.0.0 if >8.8 || 9. This actually matches the core requirements for >=1.3.

So it look like we can still have webform as a dependency.

Tests pass, I'm merging this into 8.x-1.x-dev.

Why is this requiring webform 6 for drupal 8? I would say that having to upgrade to webform 6 to use this module is quite the leap.
I would suggest to create releases that support both webform versions or if this is not required for the main module, add it as suggested in the composer.json instead.

That is a fair point, and in retrospect I think I actually agree with Villette as well.

Adding this as a suggest is a great compromise. Here's a patch for the latest version putting webform as a suggested, for testing. If this doesn't work, then I suggest recreating Villette's patch and remove it from the dependencies altogether.

Tests are failing, due to MarketingCloudExampleTest->testModuleFunctionality() (Test that the module can uninstall if the webform no longer exists), I think because it would be unable to install marketing_cloud_example in the first place.

Not sure if this would help, but you could add the key "test_dependencies" on the info file and add webform there, example: https://git.drupalcode.org/project/fraction/-/blob/2.x/fraction.info.yml, you can also leverage "require-dev" to specify which version you want, example: https://git.drupalcode.org/project/fraction/-/blob/2.x/composer.json

Following is a test patch with test_dependencies in marketing_cloud_example.info.yml.

I actually can't find a way around it.

The composer requirements can only go in the main module, not in the sub-modules. So that means whatever required modules a sub-module needs, must be defined in the ,ain composer.json file - thus must be installed when the main module is installed.

So we can have dependencies defined in the .info.yml file for a sub-module, This will give a prompt to the user of they try to enable marketing_cloud_example, and drupal 'should' pre-install the views module.

Adding it as a suggest in composer.json is a half-decent workaround.

The tests for marketing_cloud_example will have to be removed though. There is no way that I could find to pre-install the dependencies for the tests. So I suggest we go with #14.

Does everyone agree?

Please be aware that Drupal CI doesn't apply composer patches or the test dependencies , you need to push in a try and error, so I would suggest to try #17 and see if the tests pass on the Automated testing tab of the module

Hi pcambra,

As you can see, I had already tried doing tests on the patch, which failed (https://www.drupal.org/pift-ci-job/1974155):

There was 1 failure:

1) Drupal\Tests\marketing_cloud_example\Functional\MarketingCloudExampleTest::testModuleFunctionality
Unable to install modules webform, webform_ui, marketing_cloud, marketing_cloud_sms, marketing_cloud_example due to missing modules webform, webform_ui.

So I think this is a no-go...

That's not going to work, you need to push directly the changes on the composer.json and info.yml file first and then test, the CI will not do that for you, unfortunately.

Not a lot of docs but some comments here and there describing the issue https://www.drupal.org/project/drupalci/issues/3157542

I managed to do this on fraction module: https://www.drupal.org/project/fraction/issues/3097527

I've committed #17 to the dev branch, and you you're correct pcambra. Good spot.

The D9 tests only failed by an invalid test case of MarketingCloudTest::testMarketingCloudSession Failed asserting that null is false. This is expected and resolved in a separate ticket.

For testing on this, you'll need to checkout the dev version: 8.x-1.x-dev, and then install

Assuming there's no feedback on this, i'm going to mark this is ready and it'll be in the next release

Dammit, D8.9.12 tests now fail, due to:

--- Errors ---
You are using the deprecated option "--no-suggest". It has no effect and will break in Composer 3.
> Drupal\Composer\Composer::ensureComposerVersion

Here's a patch to just remove the dependency altogether

bad patch, new version