Add \Drupal::getMajorVersion()

Just an attempt.

The issue needs testing and all occurrences of something like explode('.', \Drupal::VERSION, 2) need to be replaced.

1. +++ b/core/modules/help_topics/tests/src/Functional/HelpTopicTranslatedTestBase.php
   @@ -77,10 +77,11 @@ protected function installParameters() {
   -    include_once $this->root . '/core/includes/install.core.inc';
   

   This removal seems out of scope.

2. +++ b/core/modules/help_topics/tests/src/Functional/HelpTopicTranslatedTestBase.php
   @@ -77,10 +77,11 @@ protected function installParameters() {
   +
   +//$version = \Drupal::getMajorVersion() . '.0.0';
   

   This shouldn't be here.

#8.1 Oh I see, in which case can we just replace

    include_once $this->root . '/core/includes/install.core.inc';
    $version = _install_get_version_info(\Drupal::VERSION)['major'] . '.0.0';

with

    $version = \Drupal::getMajorVersion() . '.0.0';

(which I assume was what the commented out code was trying to do?)

+++ b/core/modules/help_topics/tests/src/Functional/HelpTopicTranslatedTestBase.php
@@ -25,7 +25,7 @@ abstract class HelpTopicTranslatedTestBase extends BrowserTestBase {
-  protected function setUp() {
+  protected function setUp(): void {

This change is out of scope.

Added testing.

Added a CR for the new public method \Drupal::getMajorVersion().

+++ b/core/tests/Drupal/Tests/DrupalTest.php
@@ -0,0 +1,18 @@
+    $this->assertSame('9', \Drupal::getMajorVersion());

I can't quite decide if this is a good test or not. It will obviously break for Drupal 10 but I think that's OK, as the major version is something that doesn't change often. The alternative is just check that the result is both a string and a number, I guess.

#3292490: Use Drupal::VERSION not hard-coded integer for current major version of core reminded me about this issue, which now needs a reroll.

Updated CR to mention 9.5.0/9.5.x as introducing version/branch.

Attached reroll patch against 9.5.x

Fixed Failed commands #21

Thanks for rerolling.

It should also be possible to replace the following with this new method:

core/lib/Drupal/Core/Command/GenerateTheme.php
361:    return explode('.', \Drupal::VERSION)[0];

core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
55:    [$this->destinationSiteVersion] = explode('.', \Drupal::VERSION, 2);

Finally we will also need a separate version of the patch for Drupal 10 as this test will need updating:

+++ b/core/tests/Drupal/Tests/DrupalTest.php
@@ -0,0 +1,20 @@
+    $this->assertSame('9', \Drupal::getMajorVersion());

Addressed the comment #23
Attached interdiff

Fixed the custom command failed issue in patch #24.

#25 looks good for 9.5.x but we need a different version for 10.0.x with the test updated to look for 10 instead of 9.

Or maybe we should just check that \Drupal::getMajorVersion() returns a number?

Aded Patch against #25 in drupal 10.1.x

Thought about this one some more, and I'm not sure there's any benefit in adding a test here. Therefore #28 is RTBC.

1. +++ b/core/lib/Drupal.php
   @@ -732,4 +732,14 @@ public static function messenger() {
   +  public static function getMajorVersion() {
   

   Let's add a return typehint of string here.

2. +++ b/core/lib/Drupal/Core/Command/GenerateTheme.php
   @@ -384,7 +384,7 @@ private function isStarterkitTheme(Extension $theme): bool {
       * @return string
       */
      private function getVersion(): string {
   -    return explode('.', \Drupal::VERSION)[0];
   +    return \Drupal::getMajorVersion();
      }
   

   As this is private we can completely remove this method and use \Drupal::getMajorVersion(); instead of this method.

3. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
   @@ -301,6 +301,8 @@ protected function getCredentials() {
   +    // Remove isolation_level since that options is not configurable in the UI.
   +    unset($connection_options['isolation_level']);
   

   This is unrelated change as far as I can see.

Addressed #30.

See that a change record has been added.
Issue summary is clear what the proposed solution is.
Tests are included.
Typehints added.

Looks good!

It strikes me that given this is effectively returning a constant, why don't we just declare \Drupal::MAJOR_VERSION as a constant and use that?

@longwave it becomes something to maintain and possibly get out-of-sync? I guess we should add a test for that. It is what Symfony does - see \Symfony\Component\HttpKernel\Kernel

I think you're right - adding a constants for this makes sense - but it feels like something that needs all the RMs to agree on as this constant will be maintained by you.

I don't see an issue with it if we add a test, given that this only needs to change every two years or so.

Werenʻt we discussing getting this information from Composer? If that is performant enough.

Iʻm thinking of https://www.drupal.org/project/drupal/issues/3266523 but I think we could use a similar approach for this metadata.

@xjm if you pulled it from composer what do you pull? "drupal/core" would return "self.version" so then would still need a lookup? Maybe I'm reading that wrong.

Yes, there is nothing that I can see in Composer that can tell us the version; also, nobody is currently forced to use Composer and there is a chance they might be using some deployment mechanism that doesn't even ship a composer.json or .lock file, given they are not needed at runtime.

The canonical place to read Drupal's version number is the \Drupal::VERSION constant. As far as I can see, neither of the two version-handling libraries that are available to core - composer/semver and sebastian/version - provide a method to extract the major version number from a version string; and even if they did, I don't see the benefit in runtime code executing on a constant to calculate another constant, when we can just declare the constant and write a test to ensure it is correct.

So right now the approach is either #31 or doing something like you mentioned in #33-#35

FWIW, I think the major version constant is the best solution here, and seems like an extremely low maintenance burden, especially with the proposed test.

Second choice is the very small runtime solution we’ve already got.

Don’t want to NW to force option 1, so leaving NR.

Thanks,
-Derek

Seems like the consensus is to use a constant MAJOR_VERSION

I like it. Is there a checklist somewhere for new major version releases? Like xyz has to be done for a new major version. If so wonder if this could be added to it.

Yes, this should be added to the core committer handbook at https://www.drupal.org/about/core/policies/maintainers/create-core-branch

The script at https://github.com/xjm/drupal_core_release/blob/main/branch.sh#L35-L36 will also need updating to match.

Thanks! Mostly looks great.

I don't think we want to make the updates in #46 until this is actually accepted, so tagging for updates, instead.

1 actual point (a very minor nit) and 2 other questions:

1. +++ b/core/lib/Drupal.php
   @@ -77,6 +77,11 @@ class Drupal {
   +   * The current system version.
   

   "The current system major version." Otherwise, it's the same comment as VERSION.

2. +++ b/core/lib/Drupal.php
   @@ -77,6 +77,11 @@ class Drupal {
   +  const MAJOR_VERSION = '10';
   

   Do we want it VERSION_MAJOR so that it's alphabetically "next" to VERSION itself? I don't think we'll actually need it, but if we end up adding a constant for MINOR, would it be better to have VERSION, VERSION_MAJOR and VERSION_MINOR sorted together?

3. +++ b/core/tests/Drupal/Tests/Core/DrupalTest.php
   @@ -456,6 +456,14 @@ public function testPhpConstants() {
   +  public function testMajorVersion() {
   

   Love it, so simple and clean. Thanks!

   Do we need to see this failing in test-only patch mode, or are we sufficiently happy with visual inspection? 😅

Here's patch for point #1. NR for #2. A committer will tell us if we need #3. 😉

Thanks!
-Derek

Per @longwave in #26, looks like this is a candidate for backport to 9.5.x, so here's a patch for that.

While I was at it, I intentionally ran the test once locally before I fixed MAJOR_VERSION to match the 9.5.x branch to get a test-only fail result. Worked perfectly. Output attached here. Not sure it's worth the bot resources to do it here in the queue.

#26 was while 9.5.x/10.0.x were still in development, as this is new API this can only go into 10.1.x now.

#47.2: MAJOR_VERSION is more readable to me - I can't see why alphabetical order would be important and would prefer readability.

#47.3: I don't need to see a test fail, the test is obviously correct to me.

Removing "needs release manager review" as I think @xjm's concerns have been answered and @quietone wrote most of the new patch, so I don't think any more release managers need to be involved here. This looks ready to go to me.

Re: D9 backport: hehe, whoops, didn't look closely at the dates of the comments. 😅 That said, I find it hard to believe this would be disruptive in any way, and could potentially be useful for people. I'd personally still vote for 10.0.x and 9.5.x commits, but I tend to be on the wrong side of such debates. 😂

Re: #47.3: I provided the fail results from the local run. Agreed, the test is obviously correct.

Thanks,
-Derek

p.s. Fixing the proposed resolution in the summary.

FWIW I'm not totally convinced this is an improvement; my concerns have not been addressed because I'm not sure this approach is worthwhile. I want fewer constants etc. attached to the major and minor version metadata that we have to maintain and update, not more. We constantly have issues with this-or-that thing that needs an update being missed when we open a new branch already.

We would not backport it to Drupal 9 because it is an API addition (though admittedly a low-risk one because no one has any business subclassing \Drupal, but it isn't final or anything).

Take a look at Drupal\Core\Extension\ExtensionVersion. That uses Composer's semver regex for validating and parsing out version information. We could add a related CoreVersion class in the same namespace, and possibly provide some of the functionality in the base path. And we could also parse the Composer version out of composer.json rather than having another constant to maintain, if it's sufficiently performant and/or never in the critical path.

Where is the Drupal version in core's composer.json? There's no requirement to even have a composer.json present on disk at runtime.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

Patch for 10.1.x.

Removing credit for a reroll that does not apply. Thanks!

IGNORE

Updated patch #55 as it wasn't applying.

Following back up on this.

@xjm I also don't see the current version in the composer.json file, same as @longwave in #52

I agree with @xjm here, every time we create a new major version, there is about 2-4 days where HEAD is broken because we've hardcoded major versions somewhere, and we change all those hard-coded places to the new hard-coded versions, open follow-ups for them to make them cross-major-safe, forget about them for 2-3 years, then do it all over again. Every place we add a new hardcoded major version assumption is potentially another hour expecting HEAD to go green then finding we missed another one. Actually changing these is a minimal amount of work, but when any of them is not changed it disrupts core development for another hour or two. The test means the maximum fallout of this change is one hour or so, but that's still an hour.

However I think the patch in#31 would be fine. The method isn't as 'nice' as a constant but it cleans things up compared to now.

Extra thought: if we could do something at the phpstan level, that would fail a lot earlier, but then it feels like we're maintaining custom phpstan integration for a constant... so a one line method just seems like it ought to be least maintenance here.

Hiding patches except #31. Which still applies fine to 10.1

Marking this but @catch do we want a follow up for creating the phstan check you mentioned in #62

No, that follow up would have been to notice that the constants were out of sync earlier in a core build. Now that we’re going with a function, no additional check needed.

Fixing title before commit. Also crediting all the RMs, and @smustgrave for reviews, input, etc.

Thanks!
-Derek

Agreed that a method is better than a constant.

I don't necessarily think we should add this method to \Drupal, though. We should be reducing the size of that class, not adding to it. It would be better to add it to a core namespace (compare the extension versioning classes).

NR for #66. I think there's an opportunity to do some refactoring with existing core extension version APIs (which are based on composer/semver) to a parent class or something.

If the scope of this ticket is expanding should the issue summary be updated.

Grumble, grumble. 😉

1. This isn't about an extension, it's about core itself.
2. \Drupal\Core\Extension\ExtensionVersion is all about trying to hide the details of bespokever vs. semver contrib version strings, and includes @see https://www.drupal.org/drupalorg/docs/apis/update-status-xml to explain itself. Seems highly weird to try to fit this method into there.
3. Looking through the rest of core/lib/Drupal/Core, there's no particularly good spot I can see to add this.
4. Maybe core/lib/Drupal/Core/DrupalKernel.php? But that seems messy, and would pollute the idea of that class/interface.
5. Do we really want a whole new class in core/lib/Drupal/Core/Utility, something like CoreVersion.php for this 1 line method?

I know consensus is great when we can get it, but with 3 out of 4 RMs happy with the tiny method directly in \Drupal, at what point can we agree to disagree and be done with this? 😅

Thanks,
-Derek

That's also a fair point. How do we want to proceed?

Found some test code using _install_get_version_info() in #3359077: Resolve test failures on 11.x branch which can/should also be converted to the new helper.

@catch now sure how to best proceed here. Seem to be stuck on agreeing on a consensus.

How about I open a follow up to explore refactoring once we hit a second thing this code would share in common with the existing contrib version classes, but for now go with the simple fix under the banner of YAGNI? 😅

Back to needs work to also fix #71, I think?

There's also an issue with minor version part as core now using 11.x-dev https://3v4l.org/54Wc2

Faced in https://git.drupalcode.org/project/drupal/-/blob/11.x/core/tests/Drupal/...

So probably we need 2 methods get[Major|Minor]Version()

Per #3364646: Add a branch alias for 11.x, I don't think \Drupal::VERSION should be 11.x-dev right now in the 11.x branch. If that branch holds what's to become 10.2.x, the VERSION should be "10.2.x-dev". Therefore, I'm not sure we care about adding getMinorVersion() here to help work-around the weirdness until we've got a main branch to work from. And once we're at main, #3364646 will be even more important to be accurately communicating what version "main" is going to become next...

@dww yes, it's 11.0-dev now https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal.ph...

But there's a warning https://3v4l.org/HWFR0

[$major, $minor] = explode('.', '11.0-dev');
echo $major + 1;
echo $minor + 1;

results

12
Warning: A non-numeric value encountered in /in/HWFR0 on line 5
1

Filed separate issue for the test #3365970: Fix minor version parsing in InfoParserUnitTest.php

BTW I am aware ExtensionVersion is about extensions, but the reason I keep bringing it up is that IMO those classes should subclass something else that comes from core. And that something that comes from core should also look to the Composer metadata, and handle whatever special cases it has that Composer metadata cannot provide.

Coming in late to the conversation and want to revisit having a separate constant for MAJOR_VERSION. I understand the concern that it gets out of sync, but surely we can just add a unit test that the first part of \Drupal::VERSION matches Drupal::MAJOR_VERSION. No need for phpstan then, and it will fail early.

Or another option, instead of having a function for getting the major version, we could have major (11) and minor (3-dev) as consts and a function that combines them.