Module constraint checks fail incorrectly due to str_replace

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The code in question in ModuleDependencyMessageTrait has changed in the last three years since this issue was raised. Is this still an issue? Can we get some steps to reproduce?

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 as the problem code has been moved again.

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 a 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

Hello @jonathan1055 reading the issue summary thanks for the update!

Adding some tasks for "Remaining tasks" section.

This will need a test case to show the problem.

Thanks!

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.

Test looks good!

Can you revert the one test change and think I can mark it RTBC.

Thanks @smustgrave, will do.

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

Then try to install the 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 for all the work on this @jonathan1055

Crediting smustgrave for an issue summary update

Committed to 10.1.x and backported to 10.0.x and 9.5.x - thanks!

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

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