Problem/Motivation

At #3290808-20: Remove workspaces special casing from system_requirements() and fix versions/docs I pointed out:

+++ b/core/modules/system/system.install
@@ -1368,18 +1368,16 @@ function (callable $hook, string $module) use (&$module_list, $update_registry,
+          '@current_major' => 10,

Probably out of scope here, but why hard-code this instead of getting it from \Drupal::VERSION?

Steps to reproduce

Proposed resolution

Use \Drupal::VERSION not hard-coded integers to print out the current major version of core.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3292490

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

dww created an issue. See original summary.

immaculatexavier’s picture

Status: Active » Needs review
StatusFileSize
new1012 bytes
new1012 bytes

I've attached the patch in accordance with the proposed resolution.

spokje’s picture

Status: Needs review » Needs work

Sadly the patch doesn't apply. It looks like it was against an outdated version of 10.0.x-dev.

dww’s picture

Thanks for moving this along. Also need work since \Drupal::VERSION is the full version (eg 10.0.1). We need to parse just the major version out of that.

ankithashetty’s picture

Status: Needs work » Needs review
StatusFileSize
new1012 bytes
new1.88 KB

Rerolled the patch and addressed #4, thanks!

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks good to me.

This issue reminded me of #3134417: Add \Drupal::getMajorVersion() as well.

dww’s picture

Status: Reviewed & tested by the community » Needs review

I know I said “current” in the title, but if we’re doing this much, what about '@previous_major' 2 lines down?

longwave’s picture

If we are to do that we should move @required_min_version to a constant somewhere and then calculate @previous_major from that I think?

dww’s picture

Yeah, perhaps. I'm not sure the best way to handle this, given that part of it *does* have to be hard-coded by a human. You're probably right that a constant is better than what we have here. It seems like we should find all these hard-coded version strings peppered throughout core and unify their handling so we don't have to track them down each time we rev a major version.

This issue reminded me of another issue, too. I struggled to locate it at first, but then I found #3110223: Add ModuleVersion::getMinorVersion() method. I see that's totally different from #3134417: Add \Drupal::getMajorVersion(), so definitely nothing duplicate. Thanks for the link, following #3134417 now...

smustgrave’s picture

Version: 10.0.x-dev » 10.1.x-dev
Status: Needs review » Needs work

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200

Moving to NW for an agreed upon solution. Though sounds like a single instance is the approach.

Thanks

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mstrelan’s picture

Category: Task » Bug report

Considering \Drupal::VERSION is currently 11.3-dev and we have 10 hardcoded I guess 10 is wrong? That would make this a bug.

niranjan_panem made their first commit to this issue’s fork.

niranjan_panem’s picture

StatusFileSize
new1.26 KB

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.