Experimental modules should have their own version numbers

I think this looks great and would help satisfy the concern I raised in #2644818-7: Inline Form Errors Module Description

+++ b/core/modules/inline_form_errors/inline_form_errors.info.yml
@@ -1,6 +1,6 @@
-version: VERSION
+version: 8.1.0-unstable
 core: 8.x

Can't we do something like 'VERSION-unstable' so that we don't necessarily need to reroll this every minor release ? It would need a change in InfoParserDynamic though.

For Migrate, I think beta makes sense for Migrate itself, especially while we're still exploring the plugin implementation. alpha is good for migrate_drupal which has missing D7 upgrade paths and migrate_upgrade which still requires some UX improvements and further testing.

As Form system maintainer, unstable for Inline Form Errors makes sense.

Untagging since @benjy covered the other two.

Ha, yeah, that's a bit unfortunate, then again, one should never go in production with a alpha/beta core right ? ;)
It doesn't bother me /that/ much, but I'm not against the initial patch either, it was just a thought.

Looks like we're going to need a new replacement. I was pondering whether we to provide MAJOR, MINOR and PATCH replacements but that seems extremely complex. What we need to is \Drupal::VERSION without the stability modifier. So perhaps "VERSION-" is a good enough way to indicate this.

We could potentially remove the suffix from VERSION before adding the suffix - would just need to strip [a-z] of the end of it.

Re #13: Actually I don't think that's very complex. In fact, we do some string parsing of \Drupal::VERSION to e.g. validate the core: 8.x declaration in moduels, etc. which would be a lot saner with something like \Drupal::MAJOR_VERSION. That doesn't mean that this issue should be the one to introduce that whole logic, but just saying that in general I think that would make a lot of sense.

Unfortunately it is not possible to do proper testing without adding a method to InfoParserDynamic. But as this is for 8.1.x should be okay.

Spelling mistake.

oopsie

So I guess with the patch in HEAD we have the issue that an experimental module could be ahead of core it terms of stability. I think in that case we should just change the string to version. Or we could get clever and compare the versions and use the most unstable.

Here's a patch which makes it impossible for an experimental module to get ahead of core.

+++ b/core/modules/inline_form_errors/inline_form_errors.info.yml
@@ -1,6 +1,6 @@
-version: VERSION
+version: VERSION-unstable

Just a thought shouldn't this be -dev? Adding a test to ensure that unstable is less stable than dev...

+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -38,8 +38,17 @@ public function parse($filename) {
+          // is VERSION-beta in the info.yml file the displayed version with be

s/with/will

Wouldn't it be nice if the test runner told you've added a test that has not run :)

As much as it was fun writing more magic into the extension system I have to agree removing all the magic and making it simple feels like a big win. And it's nice this doesn't break update status.

This approach has a +1 from me as a framework manager and I would also be happy to commit this to 8.0.x as well as 8.1.x.

Leaving at needs review to get release manager and product manager review.

+++ b/core/modules/inline_form_errors/inline_form_errors.info.yml
@@ -1,6 +1,6 @@
+version: 8.x.x-unstable

Can these all be 8.y.x-whatever instead? the double x is making me twitch :)

I like this proposal but recommend that we drop the -unstable extension. To me, -unstable is ambiguous because -alpha and -beta releases for _experimental_ modules are all "unstable". It will probably not be clear to site builders whether -unstable is more or less stable than -alpha. I recommend we not add experimental modules unless they are at least alpha-quality. This way we can drop the -unstable tag. I believe that will be the easiest and clearest for site builders to understand.

@alexpott mentioned using -dev in #21. As it's the tag we use everywhere else to indicate a module is unstable and has not entered its release preparation process, it seems fitting we use it here as well.

PS: I read through http://semver.org/ again when reviewing this proposal, and realized that I can only find A normal version number MUST take the form X.Y.Z where X, Y, and Z are non-negative integers with regards to valid characters in version numbers. Seeing as it says ...normal version number..., I would expect there to be another section that explains what a non-normal number may be, but I cannot find any such thing.

Thanks @Xano. We can't use -dev, because it is too close to (e.g.) 8.0.5-dev for stable core. We need to be able to make it obvious to site owners that the version is different from core, even in -dev branches of core.

That makes sense. I missed that. Thanks!

My only concern is SemVer conformity. I do not expect this change has (m)any practical consequences, but it wouldn't be professional to not document this. If you agree, I'd be happy to do the writing. What would be a good place to document this? I couldn't find an existing handbook page yet.

I see I haven't been clear about this. The SemVer docs say A normal version number MUST take the form X.Y.Z where X, Y, and Z are non-negative integers, while this patch uses letters for Y and Z. Technically, this seems to be an invalid SemVer version number. I understand we do this for a good reason, and my point is that we should document this deviation from SemVer. I was willing to write this, but I hadn't found a good place to do so yet. https://www.drupal.org/documentation/version-info/numbers looks like the right place though. Thanks for digging that up.

+1 for alpha for migrate

Here's a patch making migrate alpha and adding a number to everything.

@xjm - what do we need to test exactly? What is wrong with core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php?

@xjm we do use version_compare() everywhere to compare versions to each other - we just manipulate the dependency strings in .info.yml files to satisfy our requirements. But this comment suggests we might need to do something for the y as well... so definitely worth testing.

Somewhat annoyingly there is no explicit test of drupal_check_incompatibility()

Well this is a can of worms because core dependencies are different from contrib dependencies. Which is leading me to think that experimental modules should look more like contrib than core - ie. have a version string like 8.x-0.x-alpha1. Then we don't need to change any dependency mangling code because this just works... Also the major version of the project would 0 which is totally compatible with semver notions of BC... ie. anything before 1 can be changed.

The reason for #64 is both ModuleHandler::parseDependency and ModulesListForm and system_requirments all do...

str_replace(\Drupal::CORE_COMPATIBILITY . '-', '', $modules[$dependency]->info['version'])

And we need this too... it strips 8.x- from the front of the version string.

While 8.x-0.x-alpha1 makes a lot of sense especially in the context where we so, its for modules we might remove again, because, well this is one of the experimental side of things, I'm a but confused what this would mean for modules, which want to require a specific version of those modules.

Let's say, they require 8.x-0.x-alpha2, no the core module is no longer experimental, so its version schema changes to 8.2.x. Is this dependency then fullfillled?

Here's a patch based on #64 and with a fix to the dependency parsing to support -unstable.

There no interdiff because it is yet another approach.

Let's say, they require 8.x-0.x-alpha2, no the core module is no longer experimental, so its version schema changes to 8.2.x. Is this dependency then fullfillled

No it is not but if they require >=8.x-0.x-alpha2 it is. Which is exactly how it works now.

Oh good point.

Oh I see I've mis-understood... good question @dawehner. I'm not sure. Depending on particular versions of core modules is weird because this is normally handled by the core key in the info.yml file... the whole 8.2.0 is not handled well by ModuleHandlerDependency parser...

@dawehner we're okay it works as expected but only because we also key the module's core key in the info.yml otherwise one could do some really strange things.

The interdiff is the test only patch - I'm submitting a test only patch to show we've changed nothing about the assumptions around version checking that occur when we move a module into core. It should be green too.

+++ b/core/tests/Drupal/KernelTests/Core/Common/DrupalCheckIncompatibilityTest.php
@@ -0,0 +1,60 @@
+    // If the module version starts with 8.x- is it stripped prior to using

is it = it is ?

Let's add some tests for the -unstable identifier and fix #73.

@xjm mentioned a problem with using the contrib numbering... if bigpipe is in contrib with an 8.x-1.x version... and then it becomes core experimental... so 8.x-0.x-alpha1... it is going back in versioning... which is very very weird.

TBH. I think if a contrib module goes into core we should always choose a different name.

Update on the last comment: I think if a contrib module goes into core we should always choose a different name if it is after the X.0.0 release

if bigpipe is in contrib with an 8.x-1.x version... and then it becomes core experimental... so 8.x-0.x-alpha1... it is going back in versioning... which is very very weird

What if instead, when a contrib module goes into core, we bump its major version? So for example, big_pipe in contrib is 8.x-1.0-rc1, but in core, we want it to be alpha, so what if we make it 8.x-2.0-alpha1? That means early adopters of the contrib module move forward in their version number when updating to what's in core, which I think is reflective of the reality in this case (the core code is a forward movement relative to the contrib code).

This also benefits from the fact that using this version number ModuleHandler::parserDependency() identifies the module's major version as 0. This means we are inline with semver which states the a major version of 0 has no bc promise. Which is the reality we are trying to get across.

Do we need this? Item #9 on http://semver.org/ says "A pre-release version MAY be denoted by appending a hyphen and a series of dot separated identifiers immediately following the patch version... A pre-release version indicates that the version is unstable and might not satisfy the intended compatibility requirements as denoted by its associated normal version." So I'm unclear what the benefit of having 0 for the major version is, in cases where you also have an alpha/beta/rc suffix.

Let's say, they require 8.x-0.x-alpha2, no the core module is no longer experimental, so its version schema changes to 8.2.x. Is this dependency then fullfillled?

If we do my suggestion, then I think we'd need to make sure that if a hypothetical experimental module went into 8.1 as 8.x-3.0-alpha1, and then became not experimental in 8.2, that in fact 8.2.x is greater than 8.x-3.0-alpha1 (I don't know how the 2 and 3 get compared in that case). And also whether 8.2.x is greater than 8.x-9.0-alpha1 (comparison of 8 and 9), though that might be quite the edge case (not many contrib modules get to major versions that high).

And also whether 8.2.x is greater than 8.x-9.0-alpha1 (comparison of 8 and 9), though that might be quite the edge case (not many contrib modules get to major versions that high).

Potentially, we might want to exploit this as a feature rather than a bug. For example, suppose a contrib module wants to hack node.module. How can it indicate to UpdateManager that it's the preferred version to what's in core? Well, if core is on 8.1, then the contrib module can version its hack as 8.x-8.2. If our comparison code strips the "8.x" prefix, then the comparison would yield the contrib version (8.2) as higher than core's (8.1) until core moves to a higher number. We'd need to decide what to do in case of equality (should we prefer core's 8.2 or contrib's 8.x-8.2?), but once we decide that, contrib can accommodate either decision.

@effulgentsia - nice idea. What always complicates this is how our modules are versioned... 8.x-2.x means that the module supports Drupal 8 but its major version is 2... and what we want is a minor version of 0. It seems an excellent idea that when contrib comes into core as experimental module we keep the contrib versioning but bump the modules major version. And it follow ups we could then use this to prefer core over contrib if core has the later major version of the module. What really complicates all of this is that modules don't yet have a version that corresponds to semver. I think the solution outlined in #83 is super.

@effulgentsia++

Patch attached implements and tests @effulgentsia's suggestion and removes support for the unstable modifier.

1. +++ b/core/modules/big_pipe/big_pipe.info.yml
   @@ -2,5 +2,5 @@ name: BigPipe
   +version: 8.x-2.0-alpha1
   

   +1 for this. Saying that in my role as one of the BigPipe maintainers.

2. +++ b/core/modules/inline_form_errors/inline_form_errors.info.yml
   @@ -1,6 +1,6 @@
   +version: 8.x-0.0-alpha1
   
   +++ b/core/modules/migrate/migrate.info.yml
   @@ -2,5 +2,5 @@ name: Migrate
   +version: 8.x-0.0-alpha1
   
   +++ b/core/modules/migrate_drupal/migrate_drupal.info.yml
   @@ -2,7 +2,7 @@ name: Migrate Drupal
   +version: 8.x-0.0-alpha1
   

   0.0-alpha seems a bit strange. Why not 1.0-alpha?

@Wim Leers - 0.0 because there has been no major release of these modules for drupal 8.

Okay. But then why not just start at 1.0? I don't know of contrib modules (or any software ever actually) that have 0.0 as a version number — it'd at least have 0.1.

If we choose to go with 0.0 instead of 1.0 to stress the experimental nature even more, then that's probably fine, though it feels unneeded, almost excessive.

I could see using 0.0 versioning for modules that are almost entirely prototypes. i.e. when the code itself has a good chance of being completely ripped out and started over.

migrate_drupal is not in that category, i.e. we expect it to go from alpha to beta to rc mostly as a result of bug fixes and clean-up.

inline_form_errors might be though, in that we don't have a clear path to get the current implementation of it working consistently everywhere, so by the time it gets to beta could be unrecognisable.

Well reading http://semver.org/ it says

How should I deal with revisions in the 0.y.z initial development phase?

The simplest thing to do is start your initial development release at 0.1.0 and then increment the minor version for each subsequent release.

So starting at 0.1 sounds entirely reasonable.

I;ve opened #2688369: If core provides a more recent version of a module use it instead of contrib to discuss the preferring contrib over core problem

Oopsie... and we have tests...

#95 looks great to me. This additionally adds tests for what's discussed in #83 and #84.

This might be a bit clearer.

#98.2 Nope - it's a leftover from before #92

re #98.1 I think that @effulgentsia was suggesting using that for a hack i.e.

suppose a contrib module wants to hack node.module

.

Oh, darn, I thought this already went in.

So did I, but someone got caught out by migrate changes and I ended up pointing them here. Tagging again.

One possibility that having the composer façade enables is keeping the experimental modules in the contrib space, and adding them to core via a composer require in core's composer.json and composer.lock.

The advantages would be that the experimental module could maintain its own versioning until such time as it reached a stable level of maturity, and it would be possible to have additional maintainers for that module until it was ready to be fully rolled into core.

A disadvantage would be split issue queues (where does one go to file issues for expermental modules?).

No, let's not complicate things. We already give experimental modules their own versions (alpha/beta/rc).

Blocks #2877066: Provide a standard way for tests to find core modules without loading experimental modules, probably

Is this still relevant? I doubt it is.

This issue was discussed by the release managers. We no longer ship alphas in a tagged release which make this unnecessary. Therefor, we agreed that this can be closed as won't fix. I am doing that now.

Cheers