Move drupal_check_incompatibility() functionality to a new Dependency class and Version component

Do you really believe this is major?

On the first read of the issue I thought, why does this belong to the ModuleHandler, this feels much more like a install level code, so should rather belong into the ModuleInstaller, but there is indeed parseDependencies, so meh. When you look onto #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList though you could argue that this maybe also belongs onto the extension list ... especially, because it is potential shared code between themes and modules.

To sum things up ideally this doesn't belong onto the ModuleHandler, because the module handler should be about runtime level stuff (hooks), but meh, I just hope we can move the code to the right place, once we have the extension list.

TBH I just filed this because I was annoyed I couldn't use the data provider from the parseDependencies() test to also test this function. :)

Ha: annoyance driven development, this is quite of a thing.

Maybe we want to move the other method from ModuleHandler too?

Ideally yes, but well, ModuleExtensionList doesnt' yet exist, maybe we could create it?

Working on moving this to a VersionChecker component.

It is certainly not the case that I really enjoy this API.:

* @return string|NULL
* NULL if compatible, otherwise the original dependency version string that
* caused the incompatibility.

For some reason "do not test" was selected by default.

I like this patch.

1. +++ b/core/lib/Drupal/Core/Extension/VersionChecker.php
   @@ -0,0 +1,104 @@
   +
   +/**
   + * @file
   + * Contains \Drupal\Core\Extension\VersionChecker.
   + */
   
   +++ b/core/tests/Drupal/Tests/Core/Extension/VersionCheckerTest.php
   @@ -0,0 +1,93 @@
   +
   +/**
   + * @file
   + * Contains \Drupal\Tests\Core\Extension\VersionCheckerTest.
   + */
   

   Don't need this :)

2. +++ b/core/lib/Drupal/Core/Extension/VersionChecker.php
   @@ -0,0 +1,104 @@
   +
   +class VersionChecker {
   

   Needs a class comment.

3. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -660,51 +660,10 @@ protected function verifyImplementations(&$implementations, $hook) {
      public static function parseDependency($dependency) {
   

   Deprecate for 9 too? And given there are only two usages I think we could do the replacement with VersionChecker::parseDependency() in this issue.
   

+++ b/core/tests/Drupal/Tests/Core/Extension/VersionCheckerTest.php
@@ -0,0 +1,93 @@
+
+  public function providerTestCheckIncompability() {

Doesn't hurt to have a short comment like the provider below.

One thing I'm wondering is: should this actually be a normal class, that can be extended for example.
Just in general this API will never be called on runtime, so worrying about microperformance doesn't really work.

.

Let's replace the two usages in core.

Some more places.

One thing I'm wondering is: should this actually be a normal class, that can be extended for example.
Just in general this API will never be called on runtime, so worrying about microperformance doesn't really work.

Im not sure what the difference would be, but I would say that this API does get called at runtime by modules on drupal.org, namely the project_dependency and project_composer modules, (parseDependency moreso than the drupal_check_incompatibility stuff).

In any case, moving this to its own versionchecker class++

+++ b/core/includes/common.inc
@@ -1197,16 +1197,13 @@ function debug($data, $label = NULL, $print_r = TRUE) {
+ * @deprecated in Drupal 8.1.0 and will be removed before Drupal 9.0.0. Use

Minor nit: Should be 8.2.0

Also, should this need to happen first before we make a patch for : #2641658: Module version dependency in .info.yml is ineffective for patch releases ?

Also, should this need to happen first before we make a patch for : #2641658: Module version dependency in .info.yml is ineffective for patch releases ?

Well we could, at least adding something like that to the module handler is kinda wrong at that point in time.

Thank you @Mixologic for the review!

I also added a change record

Thinking hypothetically here, if this were a "normal class", and drupal.org was upgraded to drupal 8, and we had need of a parser that handled all of drupal's possible version numbering schemes, past or present, would that allow us to exend this? Or is it extendable in its current state?

Thinking hypothetically here, if this were a "normal class", and drupal.org was upgraded to drupal 8, and we had need of a parser that handled all of drupal's possible version numbering schemes, past or present, would that allow us to exend this? Or is it extendable in its current state?

At least the parseDependency method is quite helpful and generic. Whether its actually useful for you, I'm not sure about that, but for now this method doesn't have any restrictions in that area. On the other hand, all that code already exists in the wild as well as functions you can just copy.

Are you looking for some composer library or so?

Mostly my musings were supposed to provide some use cases to help answer the question you were asking earlier in #12

One thing I'm wondering is: should this actually be a normal class, that can be extended for example.

I havent looked closely enough at everything to know the answer, just throwing some ideas out to see if that would be helpful.

Well, you can do the same with static methods, its just more convenient when you have a class.

Reroll.

Also, project_composer basically has re-implemented all this logic in what might be a better way. See #2641658-21: Module version dependency in .info.yml is ineffective for patch releases

So moving this to Component would be a good way to go regardless, and then as a follow-up fold in the improvements from project_composer.

@Mile23
So do you want to move this to Component? I certainly like that, even i'm not sure this version limitation is maybe just specific to Drupal itself?

You know all those tests we made to catch errors in moving something to Component? I think I hit all of them. :-)

I also ran into trouble with InstallerRedirectTraitTest but I'm not sure it's related to the changes here.

Here's a patch for the testbot to chew on while I run the tests without the changes and see if it is related.

Anyway: The substantive change here is that VersionChecker is now under Components.

InstallerRedirectTrait fixed here: #2845662: InstallerRedirectTraitTest fails under PHP 7.0.0, passes PHP 5.6.10

+++ b/core/includes/common.inc
@@ -1197,16 +1198,13 @@ function debug($data, $label = NULL, $print_r = TRUE) {
+ * @deprecated in Drupal 8.3.0 and will be removed before Drupal 9.0.0. Use
+ *   \Drupal\Core\Extension\ModuleHandler::checkIncompatibility() instead.

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -655,51 +656,13 @@ protected function verifyImplementations(&$implementations, $hook) {
+   *
+   * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0.
+   *   Use \Drupal\Core\Extension\VersionChecker::parseDependency() instead.
    */

I guess we need to update this number @_

+++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
@@ -0,0 +1,104 @@
+   * Parses a dependency for comparison by drupal_check_incompatibility().
...
+    $p_core = '(?:' . preg_quote(\Drupal::CORE_COMPATIBILITY) . '-)?';

Hmm. That doesn't look like a Component. :-)

We could make a lot of changes to this code and turn it into a component that allows other projects to resolve Drupal extension dependencies. It might be nice to have such a thing, because the composer facade could use it, or an external CLI like drush might. (OTOH, the composer facade already knows how to do this, so we might import it into core. See #2641658-21: Module version dependency in .info.yml is ineffective for patch releases)

But for consistency's sake we should probably leave this Core and not move it to Component. That way core can move forward on #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning and others.

This patch is a re-roll of #25 with CS changes and updates from #32. #27 only moves the code to the component namespace, without behavioral changes.

I guess it might be nice to include the patches.

I couldn't repro the Drupal\Tests\aggregator\Functional\DeleteFeedItemTest fail, but VersionCheckerTest needed to use PHPUnit\Framework\TestCase instead of the underscore version.

Should we also add @trigger_error statements here?

Added @trigger_error(), linked to CR, updated tests so they don't explode on @trigger_error().

According to the CR, this is just a 1:1 map of existing methods:

Before:

$v = \Drupal::moduleHandler()->parseDependency($dependency);
drupal_check_incompatibility($v, $current_version);

After:

$v = VersionChecker::parseDependency($dependency);
VersionChecker::checkIncompatibility($v, $current_version);

Now that we're finally separating this out into its own class, can we finally also take the opportunity to update things here? drupal_check_incompatibility() has never really made much sense and is usually creates a semantically backwards "flow" whenever its used in code (e.g. returns NULL if compatible). Plus its still using "arrays".... ugh.

I think it would make more sense to create a standalone Dependency class rather than a "version checker". This ensures that we have a standardized object to work with. I also think that we should create a separate Version class since that has entirely separate logic/assumptions that go with it.

Then we can compare both versions and dependencies separately or together as needed:

$version = Version::create($current_version);
$dependency = Dependency::create($extension_dependency);

// Standalone Version objects do the actual comparisson (string or Version object).
$version->compare('1.2.3', '>=');

// Dependencies can then determine if a version compatability in a truthy way (string or Version object).
$dependency->isVersionCompatiable($version);

So with the above "after" example, this would turn into:

$incompatible = !Dependency::create($dependency)->isVersionCompatiable($current_version);

Of course, if places where the machine name of the dependency is also needed, it could be done in the following way:

$d = Dependency::create($dependency);
$name = $d->getName();
$incompatible = !$d->isVersionCompatiable($current_version);

@markcarver: I think that's the right instinct. If the eventual goal is to use semantic versioning on extensions (which it seems to be) then we should eventually normalize on composer/semver. We could make a shim to convert Drupalisms to semver-useable data structures for comparison.

That should be a follow-up, so it's easier to test the new behavior without having to include an .inc file.

And yah: *arrays.* :-)

That's all well and good, but I'm just saying that I don't think it behooves us to simply create a 1:1 map of procedural code to simply stick into an arbitrary VersionChecker class (maybe its just the name... IDK).

In my mind, if we're going to create classes and move procedural code into them, then let's do it right from the get-go.

Even if LegacyVersion (or something similar) is the first class that is created here, that's still better than returning arrays.

All Dependency should really really care about is matching against a class that implements VersionInterface.

The follow-up you mention could definitely be SemanticVersion, for example, if/once we decide to go that route.

We do things in small chunks because that stands a much better chance of being understood, reviewed, and accepted, one small step at at time. If you go for the gold, and try and 'do everything right', you may land in a place of perfection, but it'll never make it in, regardless of how correct or just it might be, simply due to the limited time and attention span folks have for ensuring that things work as intended without any unintended consequences.

Given that we now have semver in core, we can actually discard the "do it right from the get-go" mindset that used to be required because you only had one shot at the next massive release. Now we can work on stuff, and it actually goes in, and it actually sees the light of day within six months.

So, Im +1 on the first step, and then +1 on the followup, which, your suggestions are totally sane, and reasonable, and the right way to go. We just shouldnt sprint the whole mile its gonna take to get there.

Again... not really point I'm trying to make here.

The "small chunks" argument really only flies or makes sense when it involves extremely complex sub-systems. These are literally just two, relatively small, procedural functions (in separate places). Please don't patronize me. I know how core works; it's why I had to take a sabbatical from it.

These should be converted into proper type based objects, to begin with, because they are so simple and because they do very specific types of logic (which should be unit tested... separately).

Just by creating VersionChecker, it adds this class to core's technical debt and won't be able to be removed until 9.x, regardless of when it becomes "officially" deprecated.

Which, as you have stated, is likely to happen a lot sooner than later. So what is the point of adding this class if it's likely to get replaced soon anyway?

When we do end up with something "better", we will have created a duplicate BC chain from drupal_check_incompatibility() to VersionChecker to "something better". This isn't ideal nor is it good planning.

Which brings me to kind of my entire point here: we shouldn't move things into a class just for "the sake of X". We should be evaluating what the old procedural code is attempting to "solve" and figure out if there is a better [8.x] solution, make a plan, and then move forward with that.

We should always avoid adding a relatively useless "1:1 mapping class". That has rarely served us well in the past unless its explicit purpose is to provide BC support (which this isn't).

I'm really not trying to be argumentive here. I was just offering a better solution than simply doing a c&p + rename to some arbitrary class.

I didn't change the status to CNW is that I understand that some of y'all might not see it this way and I don't want to hold up the issue... but...

I just felt it was necessary to bring attention to how much of a PITA drupal_check_incompatibility() has been in the past.

I believe it's a mistake to continue down a path that doesn't serve any real value other than "getting it out of an include file", which isn't enough of a reason to simply push through an issue.

Try a patch if you'd like.

Apologies If I came across as patronizing, its just that this particular issue is a hard blocker for a very important community need, that of semantic versioning in contrib, and this function is only used twice in core, and likely doesnt have a lot of uses in contrib - the only example I'm able to find is project_dependency, on drupal.org.

Can you help me understand

how much of a PITA drupal_check_incompatibility() has been in the past.

PITA for who? Where else is this getting used?

Nevertheless, drupal_check_incompatibility() exists now, is part of the public api, and therefore has to continue to return its less than ideal array structure on the off chance that some contrib or custom code is currently reliant on it. We cannot remove that until drupal 9 without potentially breaking somebodies site.

In both usages in core, we do this str_replace to pass in a version that can actually be checked. Is there any reason we couldnt refactor that to live inside of the checkIncompatibility method? It sorta changes the api, but arguably the api wouldnt work without it.

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -310,7 +311,7 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
+        if ($incompatible_version = VersionChecker::checkIncompatibility($version, str_replace(\Drupal::CORE_COMPATIBILITY . '-', '', $modules[$dependency]->info['version']))) {

+++ b/core/modules/system/system.install
@@ -817,7 +818,7 @@ function system_requirements($phase) {
         $version = str_replace(\Drupal::CORE_COMPATIBILITY . '-', '', $required_file->info['version']);
...
+        $compatibility = VersionChecker::checkIncompatibility($requirement, $version);

In both usages in core, we do this str_replace to make checkIncompatibility actually work. Would it make sense to refactor this into the method itself? Seems like the method needs the strings in a stripped format, would it be harmful to strip them inside the method?

Reroll of #39, updated deprecation messages for 8.7.0.

its just that this particular issue is a hard blocker for a very important community need, that of semantic versioning in contrib

Assuming this is true, raising this to Major. However, can the issue summary be updated to explain why it's a blocker for that? I'm assuming there's something in contrib or on drupal.org that is incapable of loading common.inc and calling a global function that's defined there? Whether that's the reason or something else, it would be good to document it in the issue summary.

This issue still has the "Needs change record updates" tag. What in the CR needs to be updated?

This issue is a bit of a mess because the process of trying to make core do semver is also a mess. :-)

It's a blocker for #1612910-384: [policy, no patch] Switch to Semantic Versioning for Drupal contrib extensions (modules, themes, etc)

I think the idea here was to put version-check-y stuff in one place. In other issues like #2641658-30: Module version dependency in .info.yml is ineffective for patch releases we'd make it a component in order to have some exportable maintained code that does the work of converting between Drupal-flavored and semver for use by d.o and others.

It needs a change record update if we're not going to make VersionChecker part of a component.

1. +++ b/core/includes/common.inc
   @@ -1197,16 +1198,13 @@ function debug($data, $label = NULL, $print_r = TRUE) {
   + * @deprecated in Drupal 8.3.0 and will be removed before Drupal 9.0.0. Use
   

   8.7.x now

2. +++ b/core/includes/common.inc
   @@ -1197,16 +1198,13 @@ function debug($data, $label = NULL, $print_r = TRUE) {
   +  return VersionChecker::checkIncompatibility($v, $current_version);
   

   guess it need trigger deprecation error

3. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -655,51 +656,13 @@ protected function verifyImplementations(&$implementations, $hook) {
   +   * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0.
   

   8.7.x

can the issue summary be updated to explain why it's a blocker for that?

Issue summary annotated with explanations.

Not sure what #51 is referring to...

Turned this into a component, because we had some consensus that was a good idea. I put it in a component called Extension, but there might be a better place. Adding it to Utility means other consumers will end up with all the dependencies there, such as drupal/core-render, even if they don't need them.

Turning it into a component necessitated adding an optional argument to parseDependency(), since components should never rely on core being present.

+++ b/core/includes/common.inc
@@ -8,6 +8,7 @@
+use Drupal\Component\Extension\VersionChecker;

@@ -1212,16 +1213,15 @@ function debug($data, $label = NULL, $print_r = TRUE) {
+ * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use
+ *   \Drupal\Core\Extension\VersionChecker::checkIncompatibility() instead.
...
+ * @see \Drupal\Core\Extension\VersionChecker::checkIncompatibility
...
+  @trigger_error(__FUNCTION__ . ' is deprecated. Use \Drupal\Core\Extension\ModuleHandler::checkIncompatibility() instead. https://www.drupal.org/node/2756875', E_USER_DEPRECATED);

Got the messages all wrong, in different ways!

1. +++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
   @@ -0,0 +1,127 @@
   +      list($project_name, $dependency) = explode(':', $dependency);
   

   should we use the third argument here e.g.
   explode(':', $dependency, 2)
   I realise this is just moved code

2. +++ b/core/tests/Drupal/Tests/Component/Extension/VersionCheckerTest.php
   @@ -0,0 +1,92 @@
   +  public function dependencyProvider() {
   
   +++ b/core/tests/Drupal/Tests/Core/Extension/DeprecatedModuleHandlerTest.php
   @@ -0,0 +1,51 @@
   +  public function dependencyProvider() {
   

   should these share a common trait?

3. +++ b/core/tests/Drupal/Tests/Component/Extension/VersionCheckerTest.php
   @@ -0,0 +1,92 @@
   +      'system' => ['system', ['name' => 'system']],
   +      'taxonomy' => ['taxonomy', ['name' => 'taxonomy']],
   +      'views' => ['views', ['name' => 'views']],
   +      'views_ui(8.x-1.0)' => ['views_ui(8.x-1.0)', ['name' => 'views_ui', 'original_version' => ' (8.x-1.0)', 'versions' => [['op' => '=', 'version' => '1.0']]]],
   +      // Not supported?.
   +      // array('views_ui(8.x-1.1-beta)', array('name' => 'views_ui', 'original_version' => ' (8.x-1.1-beta)', 'versions' => array(array('op' => '=', 'version' => '1.1-beta')))),
   +      'views_ui(8.x-1.1-alpha12)' => ['views_ui(8.x-1.1-alpha12)', ['name' => 'views_ui', 'original_version' => ' (8.x-1.1-alpha12)', 'versions' => [['op' => '=', 'version' => '1.1-alpha12']]]],
   +      'views_ui(8.x-1.1-beta8)' => ['views_ui(8.x-1.1-beta8)', ['name' => 'views_ui', 'original_version' => ' (8.x-1.1-beta8)', 'versions' => [['op' => '=', 'version' => '1.1-beta8']]]],
   +      'views_ui(8.x-1.1-rc11)' => ['views_ui(8.x-1.1-rc11)', ['name' => 'views_ui', 'original_version' => ' (8.x-1.1-rc11)', 'versions' => [['op' => '=', 'version' => '1.1-rc11']]]],
   +      'views_ui(8.x-1.12)' => ['views_ui(8.x-1.12)', ['name' => 'views_ui', 'original_version' => ' (8.x-1.12)', 'versions' => [['op' => '=', 'version' => '1.12']]]],
   +      'views_ui(8.x-1.x)' => ['views_ui(8.x-1.x)', ['name' => 'views_ui', 'original_version' => ' (8.x-1.x)', 'versions' => [['op' => '<', 'version' => '2.x'], ['op' => '>=', 'version' => '1.x']]]],
   +      'views_ui( <= 8.x-1.x)' => ['views_ui( <= 8.x-1.x)', ['name' => 'views_ui', 'original_version' => ' ( <= 8.x-1.x)', 'versions' => [['op' => '<=', 'version' => '2.x']]]],
   +      'views_ui(<= 8.x-1.x)' => ['views_ui(<= 8.x-1.x)', ['name' => 'views_ui', 'original_version' => ' (<= 8.x-1.x)', 'versions' => [['op' => '<=', 'version' => '2.x']]]],
   +      'views_ui( <=8.x-1.x)' => ['views_ui( <=8.x-1.x)', ['name' => 'views_ui', 'original_version' => ' ( <=8.x-1.x)', 'versions' => [['op' => '<=', 'version' => '2.x']]]],
   +      'views_ui(>8.x-1.x)' => ['views_ui(>8.x-1.x)', ['name' => 'views_ui', 'original_version' => ' (>8.x-1.x)', 'versions' => [['op' => '>', 'version' => '2.x']]]],
   +      'drupal:views_ui(>8.x-1.x)' => ['drupal:views_ui(>8.x-1.x)', ['project' => 'drupal', 'name' => 'views_ui', 'original_version' => ' (>8.x-1.x)', 'versions' => [['op' => '>', 'version' => '2.x']]]],
   

   can we wrap these as per the coding standards? will be a bit easier to read/review diffs in future

4. What's up with this one?

   +      // Not supported?.
   +      // array('views_ui(8.x-1.1-beta)', array('name' => 'views_ui', 'original_version' => ' (8.x-1.1-beta)', 'versions' => array(array('op' => '=', 'version' => '1.1-beta')))),

#55.1: In that section of code we're only removing the project name from $dependency and storing it.

#55.2: Similar but different data, so it's not portable.

#55.3: Done.

#55.4: It looks like ModuleHandler::parseDependency() can support views_ui(8.x-1.1-alpha12) but not views_ui(8.x-1.1-beta) because the regex wants the last character to be a digit. This gets back to the inflexibility of this code and why we need to update it in other issues. So let's make a follow-up for that: #3001344: ModuleHandler::parseDependency() can deal with beta1 but not beta

'needs followup' was in relation to having a semver shim issue, which is here (and other places): #1612910: [policy, no patch] Switch to Semantic Versioning for Drupal contrib extensions (modules, themes, etc)

Updated CR.

Had a quick scan and found 1 nit:

+++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
@@ -0,0 +1,129 @@
+   *   An associative array with three keys:
+   *   - 'name' includes the name of the thing to depend on (e.g. 'foo').
+   *   - 'original_version' contains the original version string (which can be
+   *     used in the UI for reporting incompatibilities).
+   *   = 'project' is the project for the dependency. This will be provided if
+   *     it's present in $dependency.
+   *   - 'versions' is a list of associative arrays, each containing the keys
+   *     'op' and 'version'. 'op' can be one of: '=', '==', '!=', '<>', '<',
+   *     '<=', '>', or '>='. 'version' is one piece like '4.5-beta3'.

I read 4 keys. Is it a standard to use '=' for optional keys?

Good catch. :-)

If/when this goes in, it would be great to expand it to handle all historical drupal versions, such that drupal.org can use this component when dealing with legacy projects. But I guess it premature to open a follow-up until it actually exists.

What we need here is a well-stated plan/meta. #1612910: [policy, no patch] Switch to Semantic Versioning for Drupal contrib extensions (modules, themes, etc) is up to 407 comments.

Just nitpicks, nothing major in here.

1. +++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
   @@ -0,0 +1,129 @@
   +   * @return string|null
   

   Is null correct here?

   I think I'd prefer to have FALSE instead. I think that more correctly contains the meaning of what we return.

   If ya'll agree, we should also update the code to reflect this :)

   This will probably make the patch harder to implement though, so probably not a great idea.

2. +++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
   @@ -0,0 +1,129 @@
   +   * Parses a dependency for comparison by drupal_check_incompatibility().
   

   by ::checkIncompatility?

3. +++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
   @@ -0,0 +1,129 @@
   +   *   A dependency string, which specifies a module dependency, and optionally
   +   *   the project it comes from and versions that are supported. Supported
   +   *   formats include:
   

   I think this also supports themes right? We should probably mention that. Or remove the word module entirely, I think dependency conveys enough meaning.

4. +++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
   @@ -0,0 +1,129 @@
   +    $core_compatibility = empty($core_compatibility) ? static::getCoreCompatibility() : $core_compatibility;
   

   I don't like using empty like that, but that is just pure personal preference. I prefer seeing $core_compatiblity === NULL ? $core_compatibility : static::...

   Not just because it removes the empty on a non-iterable thing, but also because it tests for the positive instead of the negative case.

5. +++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
   @@ -0,0 +1,129 @@
   +    if (strpos($dependency, ':') !== FALSE) {
   +      list($project_name, $dependency) = explode(':', $dependency);
   +      $value['project'] = $project_name;
   +    }
   

   This could lead to trouble when there are multiple colons in $dependency. Should we guard for that by throwing an exception when there is > 1? Or by doing explode(':', $dependency, 1);

6. +++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
   @@ -0,0 +1,129 @@
   +    if (isset($parts[1])) {
   ...
   +        if (preg_match("/^\s*$p_op\s*$p_core$p_major\.$p_minor/", $version, $matches)) {
   +          $op = !empty($matches['operation']) ? $matches['operation'] : '=';
   +          if ($matches['minor'] == 'x') {
   

   This code isn't super complicated, but it looks like that because of the nesting and use of regexes.

   We can't really do anything about the use of the regex, but we can improve the nesting by reversing them.

   if (!isset($parts[1]) {
     return $value;
   }
   
   foreach (...) {
     if (preg_match(...)) {
       if ($matches['minor'] !== 'x') {
          return $value;
       }
       ...
     }
   }
   
   

   It looks like this might end up with just an open bracket, and no closing bracket, are we testing for that usecase?
   It also looks weird that the original version always starts with a space, I think that should be the worry of the UI code, not this code.

This is a real nice patch.

1. +++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
   @@ -0,0 +1,129 @@
   +   * @return array
   +   *   An associative array with three keys:
   +   *   - 'name' includes the name of the thing to depend on (e.g. 'foo').
   +   *   - 'original_version' contains the original version string (which can be
   +   *     used in the UI for reporting incompatibilities).
   +   *   - 'project' is the project for the dependency. This will be provided if
   +   *     it's present in $dependency.
   +   *   - 'versions' is a list of associative arrays, each containing the keys
   +   *     'op' and 'version'. 'op' can be one of: '=', '==', '!=', '<>', '<',
   +   *     '<=', '>', or '>='. 'version' is one piece like '4.5-beta3'.
   +   *   Callers should pass this structure to
   +   *   \Drupal\Component\Extension\VersionChecker::checkIncompatibility().
   

   This seems to me like a good candidate for a value object, which the deprecated procedural functions can unwrap into an array. That might be out of scope for this issue, though...?

2. +++ b/core/tests/Drupal/Tests/Component/Extension/VersionCheckerTest.php
   @@ -0,0 +1,216 @@
   +    $this->assertEquals($result, VersionChecker::checkIncompatibility($version_info, $current_version));
   

   Why not assertSame() here?

3. +++ b/core/tests/Drupal/Tests/Component/Extension/VersionCheckerTest.php
   @@ -0,0 +1,216 @@
   +    // Test greater than.
   +    $data['views_ui(>8.x-1.x)-2.0'] = [$test_cases['views_ui(>8.x-1.x)'][1], '2.0', NULL];
   +    $data['views_ui(>8.x-1.x)-1.1'] = [$test_cases['views_ui(>8.x-1.x)'][1], '1.1', ' (>8.x-1.x)'];
   +    $data['views_ui(>8.x-1.x)-0.9'] = [$test_cases['views_ui(>8.x-1.x)'][1], '0.9', ' (>8.x-1.x)'];
   +
   +    // Test Less than or equal.
   +    $data['views_ui(<= 8.x-1.x)-2.0'] = [$test_cases['views_ui(<= 8.x-1.x)'][1], '2.0', ' (<= 8.x-1.x)'];
   +    $data['views_ui(<= 8.x-1.x)-1.9'] = [$test_cases['views_ui(<= 8.x-1.x)'][1], '1.9', NULL];
   +    $data['views_ui(<= 8.x-1.x)-1.1'] = [$test_cases['views_ui(<= 8.x-1.x)'][1], '1.1', NULL];
   +    $data['views_ui(<= 8.x-1.x)-0.9'] = [$test_cases['views_ui(<= 8.x-1.x)'][1], '0.9', NULL];
   

   Should we also test greater-or-equal and not-equal, at least?

I agree with #63.1 this sounds like an ideal usecase for a valueobject, I think the rule is that we try to avoid intruducing new array-api's, but I'm not sure how that works for conversions like this.

> That might be out of scope for this issue, though

I agree that a value object should be the ultimate goal here, but I feel that would ordinarily be out of scope.

However, once we put an api out there, especially a component, we've got to support it for ever. An api is for life, not just for christmas. Or is that a puppy. You get the idea.

But on the other hand are we sure we want to mix adding a new API with a story that blocks semver support in contrib.

Personally, I'm leaning towards making this something we want to support in the future. Thoughts?

I agree with @larowlan. New APIs should be OOP. Would it be worth punting to a follow up in the 8.7.x cycle? The API isn't 'official' until there is a stable release right?

I'm going to have a crack at it.

I've added a value object as per #63.1 #64 and #65

I haven't addressed all the feedback in #63 and #64

Looks like I missed core/tests/Drupal/Tests/Core/Extension/DeprecatedModuleHandlerTest.php

Thanks Kim for keeping this moving

1. +++ b/core/includes/common.inc
   @@ -1212,16 +1213,15 @@ function debug($data, $label = NULL, $print_r = TRUE) {
   +  return VersionChecker::checkIncompatibility($v, $current_version);
   

   You'll need to retain the old return here, so will need to unpack the value object into an array

2. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,162 @@
   +  public function setOriginalVersion($originalVersion) {
   ...
   +  public function setProject($project) {
   ...
   +  public function setVersions(array $versions) {
   ...
   +  public function addVersion(array $version) {
   

   are there uses for these setters. alter hooks I guess?

1. I think this needs to be changed for callers of VersionChecker::parseDependency()
2. Yeah. I think the fluid setter approach works best when you have lots of optional properties.

> I think this needs to be changed for callers of VersionChecker::parseDependency()

Yes, in core. But we have to retain the existing functionality for contrib.

Re #74 discussed this with @larowlan in slack and confirmed VersionChecker::checkIncompatibility() is still returning a string, so no BC issue with that.

Fixed a couple more instances calling VersionChecker::checkIncompatibility() with an array instead of VersionInfo.

Great work! I looks ready except nits & probably needs test to make sure that deprecated methods still works and throw exceptions

1. +++ b/core/includes/common.inc
   @@ -1212,16 +1213,15 @@ function debug($data, $label = NULL, $print_r = TRUE) {
   +  @trigger_error(__FUNCTION__ . ' is deprecated. Use \Drupal\Component\Extension\VersionChecker::checkIncompatibility() instead. https://www.drupal.org/node/2756875', E_USER_DEPRECATED);
   
   +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -715,51 +716,17 @@ protected function verifyImplementations(&$implementations, $hook) {
   +    @trigger_error(__METHOD__ . ' is deprecated. Use \Drupal\Component\Extension\VersionChecker::parseDependency() instead. https://www.drupal.org/node/2756875', E_USER_DEPRECATED);
   

   Links needs "See https..." https://www.drupal.org/core/deprecation#how-function

   But no dot at the end!

2. +++ b/core/tests/Drupal/Tests/Component/Extension/VersionCheckerTest.php
   @@ -0,0 +1,168 @@
   +      // This data set is currently unsupported. Fix it in
   +      // https://www.drupal.org/project/drupal/issues/3001344.
   +      /*
   +      'views_ui(8.x-1.1-beta)' => [
   +        'views_ui(8.x-1.1-beta)',
   +        VersionInfo::create()->setName('views_ui')
   +          ->setOriginalVersion(' (8.x-1.1-beta)')
   +          ->setVersions([['op' => '=', 'version' => '1.1-beta']]),
   +      ],
   +       */
   
   +++ b/core/tests/Drupal/Tests/Core/Extension/DeprecatedModuleHandlerTest.php
   @@ -0,0 +1,141 @@
   +      // Not supported? Fix this in
   +      // https://www.drupal.org/project/drupal/issues/3001344.
   +      /*[
   +        'views_ui(8.x-1.1-beta)',
   +        [
   +          'name' => 'views_ui',
   +          'original_version' => ' (8.x-1.1-beta)',
   +          'versions' => [['op' => '=', 'version' => '1.1-beta']],
   +        ],
   +      ],*/
   

   should be a @todo

1. +++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
   @@ -0,0 +1,118 @@
   +    if (!empty($version_info->getVersions())) {
   +      foreach ($version_info->getVersions() as $required_version) {
   +        if ((isset($required_version['op']) && !version_compare($current_version, $required_version['version'], $required_version['op']))) {
   +          return $version_info->getOriginalVersion();
   +        }
   +      }
   +    }
   

   We don't need the initial !empty() check. If getVersions() returns an empty array, the foreach will short-circuit and we'll jump straight to the end of the function.

   Also, I wonder if we should ever NOT have an 'op' key in $required_version. If there's no operator, shouldn't it just default to = or ==?

2. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,210 @@
   +/**
   + * A value object representing version information.
   + */
   

   Can we add a @see for ::parseDependency()?

3. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,210 @@
   +class VersionInfo {
   +
   +
   +  /**
   

   Nit: There's an empty blank line here.

4. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,210 @@
   +  /**
   +   * The project for the dependency.
   +   *
   +   * This will be provided if it's present in $dependency.
   +   *
   +   * @var string
   +   */
   +  protected $project;
   

   "Project" is a Drupalism. Can this be explained a bit in this comment? Also, the comment here mentions $dependency, but in the context of a value object, it's not clear what that is.

5. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,210 @@
   +  /**
   +   * Factory for new version info instances.
   +   *
   +   * @return $this
   +   */
   +  public static function create() {
   +    return new static();
   +  }
   

   I'm on the fence about adding this, although it *does* provide a nice fluent interface...

6. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,210 @@
   +  /**
   +   * Gets the name.
   +   *
   +   * @return string
   +   *   The name.
   +   */
   

   "Gets the name"...of what? :)

7. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,210 @@
   +  /**
   +   * Sets the name.
   +   *
   +   * @param string $name
   +   *   The name.
   +   *
   +   * @return $this
   +   */
   

   Same here.

8. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,210 @@
   +  public function setName($name) {
   +    $this->name = $name;
   +    return $this;
   +  }
   

   TBH, I'm not sure we need setters in this class. I can see no reason these value objects would ever need to be mutated. If this is for the fromArray() method, we still don't need setters; in that method, we'll still be able to set protected properties of the instance directly.

9. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,210 @@
   +  /**
   +   * Gets the versions.
   +   *
   +   * @return array
   +   *   The versions.
   +   */
   +  public function getVersions() {
   +    return $this->versions;
   +  }
   

   This doc comment should explain what each version looks like.

10. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
    @@ -0,0 +1,210 @@
    +  public function addVersion(array $version) {
    

    I don't think $version should be an array of 'op' and 'version' -- this should just accept two arguments, $op and $version, and $op should default to =.

11. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
    @@ -0,0 +1,210 @@
    +  /**
    +   * Gets the values of this object as an array.
    +   *
    +   * @return array
    +   *   The array.
    +   */
    +  public function toArray() {
    +    $arr = [];
    +    if (!empty($this->getName())) {
    +      $arr['name'] = $this->getName();
    +    }
    +    if (!empty($this->getProject())) {
    +      $arr['project'] = $this->getProject();
    +    }
    +    if (!empty($this->getOriginalVersion())) {
    +      $arr['original_version'] = $this->getOriginalVersion();
    +    }
    +    if (!empty($this->getVersions())) {
    +      $arr['versions'] = $this->getVersions();
    +    }
    +    return $arr;
    +  }
    

    I don't know if we need this; it's only used by the backwards compatibility layer. Maybe we should move this code to another place (like ModuleHandler::parseDependency()).

12. +++ b/core/tests/Drupal/Tests/Component/Extension/VersionCheckerTest.php
    @@ -0,0 +1,168 @@
    +  public function providerTestCheckIncompability() {
    

    Can we also add test cases for RC, beta, greater than or equal, and less than?

13. +++ b/core/tests/Drupal/Tests/Component/Extension/VersionCheckerTest.php
    @@ -0,0 +1,168 @@
    +    // First tests the equal comparison.
    

    ? Not sure this adds anything.

Thanks for the great feedback!

#79

1. Fixed
2. Fixed

#80

1. Fixed. The addVersion() now handles the default '='
2. Fixed
3. Fixed
4. Fixed
5. I like the fluent interface. As most of the properties are optional, having them as as optional constructor args is ugly IMO
6. Fixed
7. Fixed
8. See comment 5. I think we need setters if we are using the fluent interface. Having them as constructor args with default null values is not as nice IMO.
9. Fixed
10. Fixed! Brilliant idea! Sooo much nicer interface now. I made 'op' to be 2nd arg as its optional.
11. Its only using in Module handler so moved to a protected ::convertVersionInfoToArray() method
12. Todo once I get my head around that test case. It seems there is an unnecessary nesting of the array??
13. Fixed

Patch changes naming a bit & some clean-up

+++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
@@ -0,0 +1,116 @@
+      $value->setOriginalVersion(' (' . $parts[1]);

+++ b/core/tests/Drupal/Tests/Component/Extension/VersionCheckerTest.php
@@ -0,0 +1,166 @@
+          ->setOriginalVersion(' (8.x-1.0)')
...
+          ->setOriginalVersion(' (8.x-1.1-beta)')

not sure that original version still needs to be prefixed with space, we could keep it in BC layer (toArray)

+++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
@@ -0,0 +1,116 @@
+    $p_minor = '(?<minor>(?:\d+|x)(?:-[A-Za-z]+\d+)?)';

+++ b/core/tests/Drupal/Tests/Component/Extension/VersionCheckerTest.php
@@ -0,0 +1,166 @@
+      /* @todo This data set is currently unsupported. Fix it in
+         https://www.drupal.org/project/drupal/issues/3001344.
...
+      'views_ui(8.x-1.1-beta)' => [

+++ b/core/tests/Drupal/Tests/Core/Extension/DeprecatedModuleHandlerTest.php
@@ -0,0 +1,140 @@
+      /* @todo Not supported? Fix this in
+         https://www.drupal.org/project/drupal/issues/3001344.
...
+        'views_ui(8.x-1.1-beta)',

+++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php
@@ -496,39 +496,6 @@ public function testResetImplementations() {
-      // Not supported?.
-      // array('views_ui(8.x-1.1-beta)', array('name' => 'views_ui', 'original_version' => ' (8.x-1.1-beta)', 'versions' => array(array('op' => '=', 'version' => '1.1-beta')))),

I think we should repurpose related issue to replace [A-Za-z] with allowed [alpha|beta|rc] as https://www.drupal.org/node/1015226 states about releases

EDIT not sure about deprecated "unstable"

Fixes #82 and #83

We still need to add extra test cases.

Oh, it's getting close!

1. +++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
   @@ -0,0 +1,116 @@
   +      if ((isset($required_version['op']) && !version_compare($current_version, $required_version['version'], $required_version['op']))) {
   

   $required_version['op'] is always added, so we can remove the isset() call.

2. +++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
   @@ -0,0 +1,116 @@
   +    $core_compatibility = empty($core_compatibility) ? static::getCoreCompatibility() : $core_compatibility;
   

   Nit: This can use the ?: operator for brevity: $core_compatibility = $core_compatibility ?: static::getCoreCompatibility();

3. +++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
   @@ -0,0 +1,116 @@
   +      $value->setOriginalVersion('(' . $parts[1]);
   

   It seems weird to me to pass the parentheses to setOriginalVersion(), since it's just there to support a Drupal-specific UI. IMHO, the parentheses should be added by the UI/calling code; setOriginalVersion() should just take the original version string with no parentheses.

4. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,196 @@
   +   * Can be in the UI for reporting incompatibilities.
   

   Should be "Can be used in the UI..."

5. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,196 @@
   +   * @var array
   +   */
   +  protected $versions = [];
   

   I think the type should be array[], since it's an array of arrays.

6. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,196 @@
   +   * @return $this
   +   */
   +  public static function create() {
   +    return new static();
   +  }
   

   Nit: Technically, this method returns static, not $this.

7. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,196 @@
   +   * @return array
   +   *   The required versions.
   +   */
   +  public function getVersions() {
   

   The return type should be array[].

8. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,196 @@
   +  /**
   +   * Sets the required versions.
   +   *
   +   * @param array $versions
   +   *   The required versions.
   +   *
   +   * @return $this
   +   */
   +  public function setVersions(array $versions) {
   

   Can we add a @see referring to the $versions property, so it's clear what $versions should contain? Also, the parameter type of $versions should be array[] since it's an array of arrays.

9. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,196 @@
   +  /**
   +   * Creates a new instance of this class from an array.
   +   *
   +   * @param array $version
   +   *   The version info as an array.
   +   *
   +   * @return $this
   +   */
   

   Can we document what we would expect the $version array to look like? Also, this is returning static, not $this.

10. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
    @@ -715,51 +717,43 @@ protected function verifyImplementations(&$implementations, $hook) {
    +   * Converts the values of VersionInfo as an array for backwards compatibility.
    

    Should be "...VersionInfo to an array..."

11. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
    @@ -715,51 +717,43 @@ protected function verifyImplementations(&$implementations, $hook) {
    +  protected static function convertVersionInfoToArray(VersionInfo $versionInfo) {
    

    $versionInfo should be $version_info, since our coding standards use $snake_case.

Thanks for the review:

1. Fixed
2. Fixed
3. Fixed. Great improvement!
4. Fixed
5. Fixed
6. Fixed
7. Fixed
8. Fixed
9. Fixed
10. Fixed
11. Fixed. https://www.drupal.org/docs/develop/standards/coding-standards#naming say both is acceptable but to keep it consistent within the class. :-)

We still need the extra test cases as per #80.12

+++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
@@ -171,10 +173,19 @@ public function addVersion($version, $operation = '=') {
+   * The returned array structure contains the following keys:

Should be "The array structure may contain the following keys".

Other than that, looks great. RTBC from me once fixed and green on all backends.

Fixes #88

Added tests for >=, < , plus beta and rc as per the feedback in #80.12

This looks awesome. Thanks, @kim.pepper!

1. +++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
   @@ -0,0 +1,117 @@
   +  public static function parseDependency($dependency, $core_compatibility = NULL) {
   

   Why not require $core_compatibility and lose the complexity in dealing with \Drupal in component code?

2. +++ b/core/lib/Drupal/Component/Extension/VersionChecker.php
   @@ -0,0 +1,117 @@
   +    // We use named subpatterns and support every op that version_compare
   +    // supports. Also, op is optional and defaults to equals.
   +    $p_op = '(?<operation>!=|==|=|<|<=|>|>=|<>)?';
   +    // Core version is always optional: 8.x-2.x and 2.x is treated the same.
   +    $p_core = '(?:' . preg_quote($core_compatibility) . '-)?';
   +    $p_major = '(?<major>\d+)';
   +    // By setting the minor version to x, branches can be matched.
   +    $p_minor = '(?<minor>(?:\d+|x)(?:-[alpha|beta|rc]+\d+)?)';
   +    $parts = explode('(', $dependency, 2);
   +    $value->setName(trim($parts[0]));
   +    if (isset($parts[1])) {
   +      $original_version = rtrim($parts[1], ") ");
   +      $value->setOriginalVersion($original_version);
   +      foreach (explode(',', $original_version) as $version) {
   +        if (preg_match("/^\s*$p_op\s*$p_core$p_major\.$p_minor/", $version, $matches)) {
   +          $op = !empty($matches['operation']) ? $matches['operation'] : '=';
   +          if ($matches['minor'] == 'x') {
   +            // Drupal considers "2.x" to mean any version that begins with
   +            // "2" (e.g. 2.0, 2.9 are all "2.x"). PHP's version_compare(),
   +            // on the other hand, treats "x" as a string; so to
   +            // version_compare(), "2.x" is considered less than 2.0. This
   +            // means that >=2.x and <2.x are handled by version_compare()
   +            // as we need, but > and <= are not.
   +            if ($op == '>' || $op == '<=') {
   +              $matches['major']++;
   +            }
   +            // Equivalence can be checked by adding two restrictions.
   +            if ($op == '=' || $op == '==') {
   +              $value->addVersion(($matches['major'] + 1) . '.x', '<');
   +              $op = '>=';
   +            }
   +          }
   +          $value->addVersion($matches['major'] . '.' . $matches['minor'], $op);
   +        }
   +      }
   +    }
   

   I think this code should be in the value object.

3. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,207 @@
   + * A value object representing version information.
   

   This is a mutable value object. So it means you can create things that don't make any sense. Also

4. +++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
   @@ -0,0 +1,207 @@
   +  /**
   +   * Creates a new instance of this class from an array.
   +   *
   +   * The array structure may contain the following keys:
   +   *  - name: the name of the dependency.
   +   *  - original_version: the original version.
   +   *  - project: the project namespace.
   +   *  - versions: an associative array of required versions containing:
   +   *     - op: The comparison operation. Can be one of: '=', '==', '!=', '<>',
   +   *       '<', '<=', '>', or '>='.
   +   *     - version: the required version.
   +   *
   +   * @param array $version
   +   *   The version info as an array.
   +   *
   +   * @return static
   +   */
   +  public static function fromArray(array $version) {
   +    $obj = self::create();
   +    if (isset($version['name'])) {
   +      $obj->setName($version['name']);
   +    }
   +    if (isset($version['project'])) {
   +      $obj->setProject($version['project']);
   +    }
   +    if (isset($version['original_version'])) {
   +      $obj->setOriginalVersion($version['original_version']);
   +    }
   +    if (isset($version['versions'])) {
   +      $obj->setVersions($version['versions']);
   +    }
   +    return $obj;
   +  }
   

   I think this should be called createFromArray to be consistent with the create method. Also whilst the rest of the value object is fine without testing I think this method might deserve a test since it has logic.

5. Also drupal_check_incompatibility() is not backwards compatible.
6. I think we really need to heed @larowlan's API's are not for Christmas comment and think about what we're really building here.

Here's a patch takes a slightly tweaked approach. It introduces an extra value object Dependency which manages all of the dependency related info and moves all of the version stuff into VersionInfo and completely gets rid of VersionChecker. All the value objects are immutable so you can't create things that don't make sense. And the API for checking whether something is compatible with another version string from somewhere is a new \Drupal\Component\Extension\VersionInfo::isCompatible() method that returns a Boolean.

The patch also adds a BC test for drupal_check_incompatibility() thats passes against HEAD (if you remove the expected deprecations). Regardless of whether we go with this approach we need to continue to have this test.

Also we need to add test coverage for:

+++ b/core/modules/system/system.install
@@ -819,7 +821,7 @@ function system_requirements($phase) {
         $required_file = $files[$required_module];
         $required_name = $required_file->info['name'];
         $version = str_replace(\Drupal::CORE_COMPATIBILITY . '-', '', $required_file->info['version']);
-        $compatibility = drupal_check_incompatibility($requirement, $version);
+        $compatibility = VersionChecker::checkIncompatibility(VersionInfo::fromArray($requirement), $version);
         if ($compatibility) {
           $compatibility = rtrim(substr($compatibility, 2), ')');
           $requirements["$module-$required_module"] = [

This code in system_requirements(). The patch in #89 broken this because VersionChecker::checkIncompatibility() trims the brackets and spaces of the version string so $compatibility = rtrim(substr($compatibility, 2), ')'); is not going to work. I've fixed this in the attached patch.

Here's why I think immutability is important. With #89 the following code is possible:

$version = new \Drupal\Component\Extension\VersionInfo();
$version->setOriginalVersion('>8.x-1.1');
$version->setVersions([['op' => '<', 'version' => '1.1']]);
var_dump(\Drupal\Component\Extension\VersionChecker::checkIncompatibility($version, '1.2'));

Basically setVersions and setOriginalVersion contradict each other and this returns

string(8) ">8.x-1.1"

?!?!?!

Fixing the one test that is going to fail.

This separation looks great only $core_compatibility by-passing to version info parser looks strange

Patch fixes docbocks, removes lazy version info parser

The change in core/modules/system/src/Form/ModulesListForm.php fits in 8.7 (string change) - we mostly should escape each var separate, so once we touch this code anyway better to split module & its version vars

+++ b/core/lib/Drupal/Component/Extension/Dependency.php
@@ -62,7 +55,7 @@ public function __construct($name, $project, $version_string, $core_compatibilit
     $this->versionString = $version_string;

We can remove this property from Dependency too then. If we remove the lazy thing. We do lose this slight intended side effect though:

+++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
@@ -214,7 +215,7 @@ protected function getInstalledExtensionNames() {
-        $dependency_name = ModuleHandler::parseDependency($dependency)['name'];
+        $dependency_name = Dependency::createFromString($dependency, \Drupal::CORE_COMPATIBILITY)->getName();

We lose the slight optimization here of not having to parse the version stuff. But also it's interesting that we don't check version info here.

+1 to separating out the variables in the translatable string.

Just to throw this out there again, I had to redo the version parsing when I built the composer facade, and built a test for it so that it would handle *all* of the oddball edge cases that people have, over the years, put into their info and info.yml files that we see on drupal.org.

http://cgit.drupalcode.org/project_composer/tree/project_composer.module...

The test I wrote for that is here: http://cgit.drupalcode.org/project_composer/tree/project_composer.test#n54

Perhaps some of those cases can be adapted into this class?

Im not sure if we do or do not want to introduce things like adding the ~ and ^ operator or not at this juncture. It should still be BC compatible, just with expanded capability.

I think $core_compatibility could be renamed to $prefix to be less "core" specific and ++ #99

@alexpott optimization makes sense, but is only place where version info not used?

This is looking good, we need an issue summary (and title) update to reflect the new approach.
Couple of minor observations:

1. +++ b/core/lib/Drupal/Component/Extension/Dependency.php
   @@ -0,0 +1,154 @@
   +      $project = '';
   

   could we initialize this first and avoid the else?

2. +++ b/core/lib/Drupal/Component/Extension/Dependency.php
   @@ -0,0 +1,154 @@
   +   *   The "versions" key is ignored in favor of parsing the 'original_version'
   +   *   value.
   

   Do we need to mention it? There is no use of it in the method.

3. +++ b/core/lib/Drupal/Component/Extension/Dependency.php
   @@ -0,0 +1,154 @@
   +      'name' => '',
   

   This isn't needed - if name isn't present we throw an exception?

+1 on #99.

+1 to new approach. :-)

#99 feels like great and important follow-up material.

Re the lazy version parsing - here's another use-case #2855026: Installation profiles do not support project:module format for dependencies - still not sure about it. So we can always implement later if we find that the performance optimisation is worth (i'd hazard a guess that it is not).

One thought is that at some point in Drupal's future it'd be great if we offloaded all of this version checking stuff to composer and made it not our problem. But that gets into discussions about composition vs installation that would only side track the benefits of doing this now.

I've added some missing test coverage for version strings with commas. This has been supported forever we just haven't had core test coverage.

Still need to add tests for the update path code.

Adding the missing test coverage for system_requirements()

I've started to update the issue summary and whilst doing that it makes me question our class names. I feel that Dependency is a good name. It describes that it represents a dependency on a project with a name and might have requirement denoted by an operator + version string. But I feel that VersionInfo is not a great name. I would expect VersionInfo to deal with version strings like 8.x-1.1 or something and give you the ability to get major, minor or patch number and any modifiers such as beta. But thats not really what it is doing. It's dealing with an operator + version string and allowing you to check it against another string according to Drupal's specific rules.

Given the above I wonder if it should be called VersionRequirement or maybe even DrupalVersionRequirement because the logic is so Drupal specific. I also think that we might mirror the isCompatible() method up to Dependency as that would allow Dependency to support different version schemes.

Going to hold off on more issue summary and change record updates until this discussion is over.

Patch attached moves the up the isCompatible() check to the Dependency object which feels and looks nice. We still have separate logic between a dependency and version string but dependency no longer emits VersionInfo objects which looks good to me.

Also the patch goes a bit further in the BC layer front by making Dependency object implement deprecated ArrayAccess methods. This should allow us to use them in the dependency graph without breaking BC but also to fully deprecate dependency info being passed around as arrays.

Some fixes... so atleast some functional tests pass :)

Some more fixes.

Lol forgot to remove the backward compatibility function added previous that's now no longer necessary because we do the ArrayAccess thing.

Slight tweak to where some of the BC stuff is happening.

So for me the two remaining questions are:

1. Should we remove \Drupal\Component\Extension\Dependency::createFromArray() and put the logic in drupal_check_incompatibility() as this is the only non test usage. I kind think so because it's additional API that's completely unnecessary.
2. Is VersionInfo a good name - see #106 - I think not.

Done #114.1 - less new API has to be a good thing.

Realised that we should support Dependency objects in drupal_check_incompatibility() because people might reasonably be passing things in from $extension->requires like core used to do. $extension->requires now contains a list of Dependency objects. The good news is that we can revert the method back to how it was and it'll use the ArrayAccess BC layer and work nicely! Added test coverage for this situation.

@alexpott it looks awesome!

++ on naming - it needs more opinions
ArrayAccess mthods may use to throws specific exception message for each property

+++ b/core/lib/Drupal/Component/Extension/VersionInfo.php
@@ -0,0 +1,132 @@
+ * A value object representing version information.
...
+class VersionInfo {

About naming it looks more like DrupalVersionRequirement and looks like it missing "between" operation

+++ b/core/lib/Drupal/Component/Extension/Dependency.php
@@ -0,0 +1,176 @@
+  public function offsetExists($offset) {
+    @trigger_error('Array access to Dependency properties is deprecated. Use accessor method instead. See https://www.drupal.org/node/2756875.', E_USER_DEPRECATED);
...
+  public function offsetGet($offset) {
+    @trigger_error('Array access to Dependency properties is deprecated. Use accessor method instead. See https://www.drupal.org/node/2756875.', E_USER_DEPRECATED);
...
+  public function offsetSet($offset, $value) {
+    @trigger_error('Array access to Dependency properties is deprecated. Use accessor method instead. See https://www.drupal.org/node/2756875.', E_USER_DEPRECATED);
...
+  public function offsetUnset($offset) {
+    @trigger_error('Array access to Dependency properties is deprecated. Use accessor method instead. See https://www.drupal.org/node/2756875.', E_USER_DEPRECATED);

I bet Dependency should be full namespace and messages could be more specific on used method to catch usage better

DrupalVersionRequirement sounds great, another ++ for that.

Whilst reviewing this patch I've discovered a significant bug in the current code both HEAD and with the patch. It's caused by how we prepare the version string for checking compatibility against it.

The suspect code is:

        $version = str_replace(\Drupal::CORE_COMPATIBILITY . '-', '', $required_file->info['version']);
        if (!$requirement->isCompatible($version)) {

If $required_file->info['version']) is 8.x-8.x-beta1 this would mean that we'd check the version using the string beta1 where the intention is to pass 8.x-beta1. A version string of 8.x-8.x-beta1 might feel unlikely but it is very possible. Panels is on 8.x-4.x for example. I think we should move the $version = str_replace(\Drupal::CORE_COMPATIBILITY . '-', '', $required_file->info['version']); functionality into \Drupal\Component\Extension\DrupalVersionRequirement::isCompatible() so callers don't have to think about this at all.

Addressing #118 and #119.

+++ b/core/lib/Drupal/Component/Extension/Dependency.php
@@ -0,0 +1,176 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function offsetSet($offset, $value) {
+    @trigger_error('Array access to Dependency properties is deprecated. Use accessor method instead. See https://www.drupal.org/node/2756875.', E_USER_DEPRECATED);
+    throw new \BadMethodCallException('Dependency::offsetSet() is not supported');
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function offsetUnset($offset) {
+    @trigger_error('Array access to Dependency properties is deprecated. Use accessor method instead. See https://www.drupal.org/node/2756875.', E_USER_DEPRECATED);
+    throw new \BadMethodCallException('Dependency::offsetUnset() is not supported');
+  }

Should we implement these for full BC? I'm inclined to not because I don't think modules should have been messing with information in ->requires but someone somewhere probably has. The upgrade path would be to replace the entire value object instead of messing with the internals.

I've updated the change record - https://www.drupal.org/node/2756875

Opened a followup for #119 - see #3004174: Dependency version checking breaks for contrib versions like 8.x-8.x-rc1

Re: DrupalVersionRequurement composer semver library calls a version string and an operator a Constraint

@Mixologic that sounds exactly like what we want - so maybe DrupalVersionConstraint. There's part of me that's really tempted to rip the patch apart once more and move \Drupal\Component\Extension\Dependency to \Drupal\Core\Extension\Dependency and to create a new Versioning component instead. And then we could call the class DrupalConstraint to reflect that is a Drupal-ism.

Looking pretty good. +1 on @mixologic's naming suggestion, so it's a little more like semver.

Versioning is probably a better name for the component. So +1 on #124, except:

Other than the fact it's a Drupalism, nothing currently in the component seems to need anything from core. So I'm inclined to say leave the whole thing in the component, so other tools can parse info files and then use the component to do comparisons the way core does.

+++ b/core/lib/Drupal/Component/Extension/Dependency.php
@@ -0,0 +1,177 @@
+/**
+ * A value object representing dependency information.
+ */
+class Dependency implements \ArrayAccess {

Maybe add some comments explaining that we implement \ArrayAccess for BC, see this CR.

I think #126 actually makes my discussion about the component name more relevant - all the info parsing stuff is in Drupal\Core\Extension and if Dependency was there too we wouldn't have to the the \Drupal::CORE_COMPATIBILITY dance.

Here's the big move around with Dependency going to Drupal\Core\Extension and a new Version component.

Questions & thoughts that arise from doing this.

• I think we should deprecate \Drupal\Component\Version\Constraint::toArray() immediately - it only exists to satisfy the BC layer
• I think maybe \Drupal\Component\Version\Constraint::isCompatible() and \Drupal\Core\Extension\Dependency::isCompatible() should change their name to ::isSatisfiedBy() - this is inline with some of Composer's SemVer language.
• Reading the new Constraint class now with the language calling it a constraint makes a lot more sense - @Mixologic++ great idea.

I went ahead and deprecated \Drupal\Component\Version\Constraint::toArray() less unnecessary internals exposed in Drupal 9 sounds good to me.

And here's the patch.

Oh and now we get the lazy constraint parsing for free.

Forget to address the comment @Mile23 is asking for in #126

I went for Version over Versioning as the new component name because according to PHPStorm's dictionary Versioning is not a word and I couldn't be bothered to argue with it.

This is fabulous. Only have one minor api suggestion, but Im not attached to it at all. Consider the following bikesheddish:

1. +++ b/core/lib/Drupal/Component/Version/Constraint.php
   @@ -0,0 +1,135 @@
   +   * @param string $core_compatibility
   +   *   Core compatibility declared for the current version of Drupal core.
   +   *   Normally this is set to \Drupal::CORE_COMPATIBILITY by the caller.
   +   */
   +  public function __construct($constraint, $core_compatibility) {
   

   I think we might consider making $core_compatibility an optional argument to the constructor, since its not a requirement that the compatibility exists in the version string.

2. +++ b/core/lib/Drupal/Component/Version/Constraint.php
   @@ -0,0 +1,135 @@
   +  private function parseConstraint($constraint_string, $core_compatibility) {
   +    // We use named subpatterns and support every op that version_compare
   +    // supports. Also, op is optional and defaults to equals.
   +    $p_op = '(?<operation>!=|==|=|<|<=|>|>=|<>)?';
   +    <strong>// Core version is always optional:</strong> 8.x-2.x and 2.x is treated the same.
   +    $p_core = '(?:' . preg_quote($core_compatibility) . '-)?';
   +    $p_major = '(?<major>\d+)';
   +    // By setting the minor version to x, branches can be matched.
   +    $p_minor = '(?<minor>(?:\d+|x)(?:-[A-Za-z]+\d+)?)';
   +    foreach (explode(',', $constraint_string) as $constraint) {
   +      if (preg_match("/^\s*$p_op\s*$p_core$p_major\.$p_minor/", $constraint, $matches)) {
   

   And if the constructor's core_compatibility is optional, so should the call to parseConstraint.

Everything else in there LGTM.

@Mixologic i'm not sure about that. Especially in the light of #3004174: Dependency version checking breaks for contrib versions like 8.x-8.x-rc1. I think we should require a core compatibility because it is quite entangled with our Drupalised version strings like 8.x-4.1-rc1. Yes the code in parseConstraint will work the same for >8.x-4.1-rc1 and >4.1-rc1 but that feels kind of by-the-by and an accident of history (I might be wrong).

I guess one option would be to make it optional on both the constructor and isCompatible() but that feels icky and something we'd need to sort out now rather than later.

Minor clean-up, I also see core compatibility as long term solution, in d9 we will need to be able to process older versions as well and not clear when contrib will use composer/semver versioning for constraints

+1 on #134 because we'll soon have a moment where someone will install a module under D8 that says core: 9.x in its info file. Core shouldn't assume it's a D8 extension.

This could be a bit of a bikeshed, like @mixologic says, so maybe do a follow-up. We can change the API a little in 8.7.x before 8.7.0 release, right?

Also still wishing Dependency was in the component... Though again it might be more obvious why later, and we can do it then.

If the eventual goal is that the contrib ecosystem will be able to release modules that are d9 compatible before d9 is released, and that those modules are simultaneously compatible with d8 so that people can upgrade to them prior to upgrading to d9, then we'll need to eliminate the idea that the core compatibility is embedded in the version string, and we'll need to ensure that contrib can use semantic versioning for drupal 9.

i.e. we never want to see a contrib module with a 9.x-a.b as its version string. If a contrib module has specific core version requirements, then that has to be expressed in a dependency on the system module, not as a version string bolt on, because we don't have a sane way of expressing 'both 8.x and 9.x' in that string.

The only reason we need the core_compatibilty atm is so we can strip it out if it happens to be there. We could be really bold and just get rid of the argument entirely by changing
$p_core = '(?:' . preg_quote($core_compatibility) . '-)?';
to
$p_core = '(?:8.x-)?';

given the assumption that we should not support any 'core compatibility strings' versions beyond 8.x in our info.yml files.

This conversation is exactly why it should be a follow-up. :-)

We already have #1612910: [policy, no patch] Switch to Semantic Versioning for Drupal contrib extensions (modules, themes, etc) and #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning where this conversation is a little stalled and a lot dependent on the current issue. I'm not sure there's a follow-up specifically for whether to deprecate the core compatibility string for D9. #1612910-399: [policy, no patch] Switch to Semantic Versioning for Drupal contrib extensions (modules, themes, etc) and subsequent conversation seems to come closest and is a policy issue.

I think the solution we have here gives us BC, and is flexible enough to mutate a bit in follow-ups. And we can fold in the wisdom from #99: #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer

@Mixologic yep I totally agree with #138 and #139. One of the reasons I separated the Dependency and Drupal\Component\Version\Constraint logic is that hopefully we'll be able to detect in Dependency when we have a SemVer constraint like 4.5.0-dev and not use our constraint logic at all but use Composer\SemVer so we don't have to maintain our own set of versioning tools.

Adding review credits for those who helped shape the patch

1. +++ b/core/lib/Drupal/Component/Version/composer.json
   @@ -0,0 +1,15 @@
   +  "name": "drupal/core-version",
   

   Are we sure we want core in the name here?

2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -715,51 +715,21 @@ protected function verifyImplementations(&$implementations, $hook) {
   +      'original_version' => $dependency['original_version'],
   +      'versions' => $dependency['versions'],
   

   should we be using the methods instead of the deprecated array access?

Normalize dot at the end of link for deprecations

@larowlan re #142
1) yes, core components always namespaced by core prefix (check other components)
2) yes we have to, because access to both keys are deprecated so I added expected deprecatations

Meanwhile for some reason tests are passed fine before patch which added 2 expected deprecations to \Drupal\Tests\Core\Extension\DeprecatedModuleHandlerTest::testDependencyParsing()

Very probably @legacy doc-comment allows to skip not declared expectations

+++ b/core/tests/Drupal/Tests/Core/Extension/DeprecatedModuleHandlerTest.php
@@ -0,0 +1,140 @@
+   * @dataProvider dependencyProvider
+   * @covers ::parseDependency
+   * @expectedDeprecation Drupal\Core\Extension\ModuleHandler::parseDependency is deprecated. Use \Drupal\Core\Extension\Dependency::createFromString() instead. See https://www.drupal.org/node/2756875
+   */
+  public function testDependencyParsing($dependency, $expected) {

here all expected deprecations should be listed

An important point about #142.2 is that the use of the deprecated array access is from a method that is also deprecated. And deprecated code is allowed to use deprecated code :)

@andypost removing the fullstop from the deprecation message is not really warranted. Currently the examples on https://www.drupal.org/core/deprecation all have a fullstop for what it's worth.

@alexpott does it makes sense to file follow-up to fix it? This fullstop at the end of link really annoying for terminals where "dot" expected as part of URL

@andypost yeah - I dunno - it'd be great to have a PHPCS rule for exception messages and @trigger_error() so we have consistency. I'm not sure I care about which way we go but the inconsistency feels wrong.

#2855026: Installation profiles do not support project:module format for dependencies adds some new usages of ModuleHandler::parseDependency(). Removed them.

@larowlan's issues in #142 have all been answered/addressed so back to RTBC!

Tagging for Drupal 9 as per all the issue this postponed.

We need a follow-up issue to add the new component to core/composer.json and to \Drupal\Tests\ComposerIntegrationTest. Or do it right here.

+1 on #151.

That was the exact right thing to do. Thanks.

Back to RTBC.

Committed 550e914 and pushed to 8.7.x. Thanks!

Published change record

👏

This might not get seen, but I'm having an issue upgrading to 8.7 and I believe it's related to this change. The `system_requirements()` method is getting an old cached version of my modules list, and the module has a `$file->requires` call that is, in 8.7 expected to be a Dependency object, but in my application, it's just an array. I basically bring my current database down against new code, and try to visit the update.php page, and get...

Error: Call to a member function getName() on array in system_requirements() (line 816 of core/modules/system/system.install).

Anyone else? Any ideas?