Problem/Motivation

This module will conflict with other commerce modules such as Commerce Checkout - we should tell users by implementing hook_requirements().

Proposed resolution

Remaining tasks

Decide which modules we are not compatible with

User interface changes

New information on the system status report

API changes

None

Data model changes

None

CommentFileSizeAuthor
#15 3038059-15.patch6.37 KBalexpott
#10 hook_requirements-3038059-10.patch1.15 KBmdupree
#5 hook_requirements-3038059-5.patch1.14 KBmdupree
#3 hook_requirements-3038059-3.patch726 bytesmdupree

Comments

alexpott created an issue. See original summary.

tculling’s picture

tculling commented
Priority: Normal » Minor
mdupree’s picture

mdupree commented
StatusFileSize
new hook_requirements-3038059-3.patch726 bytes

Patch adds hook_requirements.
Checking for Commerce module and warning the user about conflicts with Commerce Checkout.

smccabe’s picture

smccabe commented
Status: Active » Needs work

Remove the TODO, thats the only conflicting module and not something that can be done dynamically.

Also you'll want to add a status page message as well, similar to the install. It should probably flag as an error on the status page, as it won't work properly.

mdupree’s picture

mdupree commented
Status: Needs work » Needs review
StatusFileSize
new hook_requirements-3038059-5.patch1.14 KB

Remove TODO, and set's error on the status page.

smccabe’s picture

smccabe commented
Status: Needs review » Fixed

Committed, made a small change to adjust Big Commerce to BigCommerce in wording.

  • smccabe committed 2555b85 on 8.x-1.x authored by mdupree 
    Issue #3038059 by mdupree: Add hook_requirements()
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Fixed » Needs work 
+++ b/bigcommerce.install
@@ -23,3 +23,32 @@ function bigcommerce_uninstall() {
+  if (Drupal::moduleHandler()->moduleExists('commerce')) {

This is not quite right. We depend on the commerce module. We conflict with commerce_checkout. So this should be commerce_checkout.

Given that we should revert this change and add automated tests.

+++ b/bigcommerce.install
@@ -23,3 +23,32 @@ function bigcommerce_uninstall() {
+    if ($phase === 'install') {

This is during the Drupal site installer not a module install so I'm not really sure when this is going to be useful. EDIT: well apparently this is also triggered during a module install but also that means rolling this back was correct because we certainly shouldn't be warning about a dependency being installed :)

  • alexpott committed 2033bb6 on 8.x-1.x 
    Revert "Issue #3038059 by mdupree: Add hook_requirements()"

This...
mdupree’s picture

mdupree commented
StatusFileSize
new hook_requirements-3038059-10.patch1.15 KB

Changes string from commerce to commerce_checkout. comment #8

mdupree’s picture

mdupree commented
Status: Needs work » Needs review
quietone’s picture

quietone commented
Status: Needs review » Needs work 
  1. +++ b/bigcommerce.install
@@ -23,3 +23,32 @@ function bigcommerce_uninstall() {
+        t('This module will conflict with Commerce Checkout as it also provides checkout.'),

    Not fond of the 'as', I think 'because' is a better choice here. 

  2. +++ b/bigcommerce.install
@@ -23,3 +23,32 @@ function bigcommerce_uninstall() {
+        'description' => t("Big Commerce provides it's own checkout functionality which conflicts with Commerce Checkout."),

    I don't think contractions are to be used in t function.

    3. Installing bigcommerce from the command line.

    $ lando drush en -y commerce_checkout
The following extensions will be enabled: commerce_checkout
Do you really want to continue? (y/n): y
commerce_checkout was enabled successfully.                                                                                                                                                                                                          [ok]
commerce_checkout defines the following permissions: access checkout, administer commerce_checkout_flow
$ lando drush en -y bigcommerce
This module will conflict with Commerce Checkout as it also provides checkout.                                                                                                                                                                       [warning]
The following extensions will be enabled: bigcommerce
Do you really want to continue? (y/n): y

    Instead of a warning should this prevent the installation of bigcommerce in case of conflict? Is the site now broken? What action should the user take? Same applies to installing via the UI.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I agree with @quietone we probably should prevent installation. We need to make the user decide upfront what to do.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Re #12.2 - it is not a contraction it is a possessive it's so that has to be fine.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 3038059-15.patch6.37 KB

Here's a new patch that has tests. It prevent install of commerce_checkout via the UI if bigcommerce is installed. There is no way to do this the other way around. It also emits a message via hook_modules_installed because Drush does not using drupal_check_module prior to doing an install like the core UI does - see https://github.com/drush-ops/drush/issues/3669

Status: Needs review » Needs work

The last submitted patch, 15: 3038059-15.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

smccabe’s picture

smccabe commented
Status: Needs work » Reviewed & tested by the community

Reran patch and tests pass, just some previous issue with head.

Re: 12.2 - Since it's possessive, it shouldn't have the ' at all, welcome to English :P

Did the minor grammar tweak and will commit.

  • smccabe committed 7c18ef1 on 8.x-1.x authored by mdupree 
    Issue #3038059 by mdupree, alexpott, smccabe, tculling, quietone: Add...
smccabe’s picture

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

Status: Fixed » Closed (fixed)

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