Module constraint checks fail incorrectly due to str_replace

Here are screen shots showing VBO and the failed constraint test. In the workings in the I.S. above, I had the Git Deploy module enabled, and that rendered the core version as '8.8.x-dev'. Without Git Deploy, the core version is 8.8.0-dev. But in both cases, the constraint test fails.

This same behaviour is also seen when a contrib module has a direct core version constraint in its .info.yml file, such as

dependencies:
  - drupal:system (>= 8.5)

Module maintainers have been encouraged to add this type of constraint when their module is only compatible above a certain version of core. However, now at 8.8.x-dev this constraint fails.

I think the code in core/modules/system/src/Form/ModulesListForm.php, function buildRow should be able to be adjusted to fix this.

Actually this does seem to be caused by Git Deploy, inadvertantly. I do not know how I got that screen grab showing the failure when Git Deploy is not enabled, as I cannot now recreate that. At Core 8.8-dev without Git Deploy the core module versions are 8.8.0-dev and the attempted removal of '8.x-' does nothing. It is only when Git Deploy is active, and the core version is changed to 8.8.x-dev do we get this nasty side-effect of the dependency's version being reduced to 8.dev

Moving into the Git Deploy queue for discussion, however, it is core code in core/modules/system/src/Form/ModulesListForm.php which does the str_replace which causes the probelm. The if(!$dependency_object->isCompatible(str_replace(\Drupal::CORE_COMPATIBILITY . '-', '', $modules[$dependency]->info['version'])) is dubious and would appear to be dangerous, as the module versions be processed are both contrib and core. There must be a better way to do this. Maybe version_compare will cope without the str_replace being needed?

Moving this back to the Core issue queue, as it is core code which needs to be adjusted. The cause of the problem is doing a blind str_replace to remove \Drupal::CORE_COMPATIBILITY- from the version string. What was actually intended is to remove it only when it starts the string, not remove from anywhere within the string. This can easily be achieved by changing to use a simple preg_replace.

The incorrect constraint results can also be seen when trying to run update.php. Attached is an example when Views Bulk Operations was installed, but now fails the constraint check. Core 8.8 is installed and VBO needs >= 8.4 but it still fails.

The attached patch makes the required change from str_replace to preg_replace in modulesListForm.php (for the listing page) and install.php (for the update.php page)

Re-rolled following the moving of the version checking into the new ModuleDependencyMessageTrait in commit in #310 on #474684-310: Allow themes to declare dependencies on modules

This change is still relevent and should still be made. It is tighter programming, and reduces the possible chance of future problems.

I've updated the IS. It still is a problem, but will not be apparent until we get to core version 9.8.x. I will upload a new patch because ModuleDependencyMessageTrait has been moved from core/modules/system/src (in 9.1) to core/lib/Drupal/Core/Extension (in 9.5 and 10.1)

Using git_deploy I can simulate the core version being moved on to 9.8 and can then reproduce the problem. I was also looking at adding a test to the 'system' module to demonstrate what happens when the core version is 9.8.x or 10.8.x instead of 9.8.0, 10.8.0 etc.

New patch for 10.1, which also applies at 9.5

Thanks @smustgrave
I have created a new test module to simulate the version change required, and added test coverage. The test module can be moved or renamed it required. The initial push to 10.1 mr is only for the test, so it should fail.

Also for speed and reduced resource usage I have temporarily limited the tests to just one class.

Thanks @smustgrave, will do.

For manual checking, the problem can be seen via UI. Install the new dependency_version_test module (which will require local settings $settings['extension_discovery_scan_tests'] = TRUE;

Then try to install the test_module which now requires View >=9.2 but it is greyed out and fails the dependency check

On question I had was is it OK to hardcode 9.2 and 9.8.x in the test modules? I used 9.x as an example because I am hoping this fix can be back-ported to Drupal 9. We will be hit by the bug in core 9.8.x sooner than we reach 10.8.x.

Branch has a random failure so I've clicked 'do not wait for branch to pass' :-)

To answer my own question from #19 about hardcoding 9.2 and 9.8.x in the tests, there are existing tests in Drupal 10 that reference core 8.x etc, and use fixtures set up for Drupal 8, so I think this is fine to have 9.2 and 9.8.x in the new test.

Thanks @larowlan for the commit and for backporting to 9.5.x
The users of git_deploy (me included) will be grateful for this when we reach 9.8.x