Fixed blocked dependencies in Project Installer.

[laryn]

Description of the bug
When a module has a dependency and that dependency has a further dependency, it causes some UX confusion in the Project Installer.

Steps To Reproduce
To reproduce the behavior:

1. Try to install Ubercart.

Actual behavior
First it asks in an additional step to download Rules (which Ubercart depends on). Then when we do that, it asks in another separate step to download Entity Plus and Entity UI (which Rules depends on). Then it doesn't allow Ubercart to be enabled:

Expected behavior
Ideally it would download all dependencies in a single step (if that is possible). Then it would be nice to be able to enable the main module (and any dependencies would automatically be enabled/checked when that module was checked).

Related for the last step: Would be nice to see submodules for enabling as well: #2200

[klonos]

I just had this happen to me with a single level of dependencies, so I don't think that this is limited to nested dependencies...

Trying to install the Search API module (which has a dependency on entity_plus):

Then once the dependency is downloaded:

As you can see, the entity_plus dependency has been downloaded, yet the checkboxes for Search API and Search Views are not available.

[klonos]

This seems to work as expected with the Manual Crop module and its 2 dependencies:

Selecting the Manual Crop module and submitting the form also enables both of its dependencies:

And confirmed:

[laryn]

Is it related to the bug that @argiepiano fixed related to three places on dependencies? Just a guess!

[argiepiano]

The fix in dependency check (enabling the 3 places check major.minor.patch) was merged into the current dev version - it has not been released yet. @klonos which version of BAckdrop are you using? I can probably take a look at this issue tomorrow and see if it's releated.

[klonos]

I'm always using either latest 1.x freshly cloned from https://github.com/backdrop/backdrop or use demo sandboxes from https://backdropcms.org/demo. So I would say the answer is "latest" 😄

[klonos]

...yup. I just double-checked and confirmed the behaviors in my previous comments on:

• a demo sandbox (using v1.20.3)
• a fresh local installation (using latest 1.x from git)

So the issue seems to be pre-existing (i.e. not caused by #5356)

The only apparent difference between the dependencies in search_api vs manualcrop is that the dependency on the former specifies a min version, whereas the latter only specifies module names.

[argiepiano]

I've been able to replicate the issue. It only happens when you proceed to the enable step after installation, in the path admin/installer/install/enable. If, instead of enabling there, you navigate admin/modules/list, you will see the checkbox appear normally. This tells me that the issue is not with the dependency check, but rather with some logic in installer_browser_installation_enable_form(). Will check on this and report.

[argiepiano]

I have found the reason for this problem.

In your particular example, @klonos , the expected behavior was for the enable checkboxes for Search API and Search Views to appear, given that the dependency on Entity Plus >=1.0.10 was true.

The issue happens in function backdrop_check_incompatibility(), which is invoked inside installer_browser_installation_enable_form(), which builds the enable form you see.

Function backdrop_check_incompatibility() receives two arguments:

• array $dependency_info, which looks like:

array(
  'name' => 'entity_plus',
  'original_version' => '(>=1.x-1.0.10)',
  'versions' => array(
    array(
      'op' => '>=', 
      'version' => '1.0.10'
    ),
  ),
);

Especially noteworthy above is 'version' => '1.0.10'

• The second argument is $current_version, which has a value of '1.x-1.0.13', which is the current version of Entity Plus.

Then the function proceeds to invoke PHP version_compare function with the following arguments:

version_compare('1.x-1.0.13',  '1.0.10', '>=');

This returns false (expected value should have been true as version 1.0.13 > 1.0.10). The issue here is that the first argument included 1.x-, the Backdrop core version, and the second argument did not.

Proposed solution: strip the Backdrop core version out from $current_version, OR add it to $required_version['version'].

I'm leaning toward the first option. Should we open a separate issue with this?

[argiepiano]

Proposed solution: strip the Backdrop core version out from $current_version, OR add it to $required_version['version'].

Stripping the Backdrop core version is EXACTLY what's done when building the list of modules in admin/modules/list. This is done by this line:

$version_string = preg_replace('/^' . preg_quote(BACKDROP_CORE_COMPATIBILITY, '/') . '-/', '', $files[$requires]->info['version']);

in system.admin.inc line 630

I suggest we use the exact same line before invoking backdrop_check_incompatibility() in installer_browser_installation_enable_form(). That will solve the problem by using code that already works in a similar functionality.

[klonos]

Thanks for getting to the bottom of this @argiepiano 🙏🏼 ...you beat me to it 🙂

Should we open a separate issue with this?

Well, the original report for this issue reports several problems:

1. The checkbox for the main module being installed is not available to be selected after all (nested) dependencies have been downloaded.
2. Download all (nested) dependencies in a single step: I don't see how that is possible unless we:
   • either make serious changes in how we retrieve dependencies (will need to happen before the module is downloaded - most likely via api calls to GitHub, which is not ideal and has other implications)
   • or skip confirmation messages for nested dependencies, and just download without any prompt for user action (possible, but need more feedback re whether that is a good thing or not)
3. Submodules not shown in the list: fixed with [UX] Project Installer: Submodules not available to be enabled during the last step. #2200

So the answer is "it depends" 😅 ...I have a working solution for point 1 above: backdrop/backdrop#3878 (tested with Ubercart, Search API, and Manual Crop and it works as expected, with the main module being available at the end of the process)

[klonos]

I suggest we use the exact same line before invoking backdrop_check_incompatibility() in installer_browser_installation_enable_form(). That will solve the problem by using code that already works in a similar functionality.

Well, I ended up stripping the core prefix in _backdrop_version_compare_convert() instead, since the intended purpose of that function is:

/**
 * Converts a Backdrop version string into numeric-only version string.
 *

[klonos]

Stripping the Backdrop core version is EXACTLY what's done when building the list of modules in admin/modules/list. This is done by this line:

We have been discussing how the version numbers should be shown in the modules list in #5002.

I have considered using regex replacement, but after reading https://stackoverflow.com/questions/4517067/remove-a-string-from-the-beginning-of-a-string it seems that using substr() is more performant.

[argiepiano]

Whether substr or regex, it's all the same to me. I was referring to the place and moment where this stripping happens rather than the method.

[klonos]

I was referring to the place and moment where this stripping happens rather than the method.

Agreed. Please see my reasoning in the reply to your comment in the PR.

[argiepiano]

I have tested and works for me. I feel this is RTBC

[klonos]

Thank you for your feedback @argiepiano 🙏🏼

I have set the milestone candidate - bug label for this issue. If you agree, can you please set the milestone to 1.20.4, and remove the label?

[ghost]

Requested some changes in the PR.

[klonos]

Thanks @BWPanda ...both changes made sense, so I committed your suggestions.

Ready for review again.

[argiepiano]

This looks great, thanks @klonos and @BWPanda, works for me. RTBC

[argiepiano]

I don't know what's going on with the labels!!

[quicksketch]

The PR at backdrop/backdrop#3878 is great! I love simple fixes to seemingly difficult problems. Thanks @klonos, @argiepiano, @BWPanda, and @laryn! I merged into 1.x and 1.20.x.