Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The Migrate Drupal UI and it tests use the string 'Drupal 8' which need to be changed to 'Drupal 9'. This issue is for the user facing text and related tests. Changes in docs and comments can be done in another issue.
Proposed resolution
Get the current major version from \Drupal::VERSION.
Remaining tasks
Patch, Review and commit
User interface changes
Yes, display the current drupal version.
Comment | File | Size | Author |
---|---|---|---|
#36 | 3133305-35.patch | 16.85 KB | quietone |
#36 | interdiff-34-35.txt | 748 bytes | quietone |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedSomething like this.
Comment #3
quietone CreditAttribution: quietone as a volunteer commentedComment #4
quietone CreditAttribution: quietone as a volunteer commentedComment #5
daffie CreditAttribution: daffie commentedWe will now be calculating the major version 6 times. Would it not be better to add a new method to the class \Drupal. Something like:
\Drupal::getMajorVersion()
Comment #6
quietone CreditAttribution: quietone as a volunteer commentedI have cleaned up the code and reduced the number of times the major version is calculated to 3.
I prefer to not make a new method to get the major version. Using grep I found 64 instances in core (without this patch) where Drupal::VERSION is used and 4 where the major version is pulled from the string. If it is decided to make a new method let's do it in a separate issue.
Comment #8
quietone CreditAttribution: quietone as a volunteer commentedThe test failure is unrelated but there were coding standard errors.
Comment #9
longwaveI was trying to decide whether we could just drop the version number here, but as these messages are related to upgrades from older versions then I think it makes sense to keep it.
This should also be backported to 9.0.x as it could be confusing that the message asks you to install Drupal 8 when a new Drupal 9 user might be looking to migrate from D6/7.
Tagged "needs followup" to add a getMajorVersion() method, there are other similar calls in core that can be converted at the same time but it is out of scope to do here.
Comment #10
quietone CreditAttribution: quietone as a volunteer commentedFollow up made #3134417: Add \Drupal::getMajorVersion() and queued a test for 9.0.x
Comment #11
benjifisherWill this issue need an update if #3024682: Migrate UI - add human-friendly module names to the Review form is fixed first?
Comment #12
xjmComment #13
xjmThis might be "Assert that the current major version is listed", or we just don't need an inline comment. c/p error?
Rest looks good. I hesitated a bit about the explodes and array magic because my first thought was "there must be API for this". In #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates we added a
ModuleVersion
class; however, Ted specifically made it final and internal because he wanted it to not be public API. I'm still not sure about that choice; however, I guess it's out of scope here to revisit that.Comment #14
benjifisherI had the same thought. I even looked a bit in the Drupal class and in
vendor/composer/composer
and did a little googling, but I did not find anything.Personally, I would not use
explode()
, but we can save our microefficiency discussion for the child issue, which is to create an API function for this.I had a quick look at the patch from #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates. Even if it were not final and internal, I do not see any method for extracting the major version. It is mostly concerned with handling version strings like 8.x-1.3 and 8.x-3.x. Here is a little excerpt from the patch:
Comment #15
daffie CreditAttribution: daffie commentedThese 2 lines can be removed on commit.
All points of @xjm are addressed.
We have a followup for the to be created method "getMajorVersion()".
Back to RTBC.
Comment #16
xjmYeah me too! I read the whole semver namespace there and TBH it's kind of a mess. 😲But nothing to help us there.
No, we don't require committers to change patches on commit -- that means unreviewed changes going into core. A stray newline or comma is one thing, but deleting whole lines should go through the review process. Please create a new patch. :) Thanks!
Comment #17
quietone CreditAttribution: quietone as a volunteer commented#15 Fixes. Comments removed.
@daffie, why should these comments be removed?
Comment #18
daffie CreditAttribution: daffie commentedHi @quietone: You removed the wrong lines. You need to remove the 2 lines where the major version is NOT "calculated". See the comment #13. My apologies, I should have communicated my previous comment more clearly.
Comment #19
quietone CreditAttribution: quietone as a volunteer commented@daffie, thanks.
The patch in #17 is bad, just ignore it. I started again from the patch in #8
Comment #20
daffie CreditAttribution: daffie commentedThank you @quietone for the new patch. See my apology in comment #18.
Back to RTBC.
Comment #21
xjmSorry that I didn't notice this before, but the version should use a plain
@placeholder
rather than a:url_placeholder
.Sorry!
Comment #22
quietone CreditAttribution: quietone as a volunteer commented@xjm, that's OK. I could use the incentive to learn how to use those placeholders correctly.
All points in #21 fixed.
Comment #23
daffie CreditAttribution: daffie commentedThe 4 placeholders are changed from ":placeholder" to "@placeholder".
Back to RTBC.
Comment #26
xjmCommitted to 9.1.x, and cherry-picked to 9.0.x (where the bug also exists) and 8.9.x (which is supposed to have the same API as 9.0.x). It's mostly just a string change, which is a beta-eligible change (aside from the new protected property, which should be safe enough before RC as well).
Thanks!
Comment #29
xjmWhoopsidaisy, this syntax isn't available on PHP 7.0. (How soon one forgets.)
Reverted from 8.9 and marking PTBP for a 7.0-friendly version. 9.0+ is fine because we require 7.3+ there.
Comment #33
xjm...And actually reverting the 8.9.x commit rather than the 9.0.x one; sorry for the noise.
(...Let's just recommit the patch on 9.0.x and pretend the other three or so reverts didn't happen, k?)
Comment #34
quietone CreditAttribution: quietone as a volunteer commentedAnd now a patch for 8.9.x.
Comment #35
daffie CreditAttribution: daffie commentedNitpick: Can we undo this change.
Comment #36
quietone CreditAttribution: quietone as a volunteer commentedOh, that is silly mistake.
Comment #37
daffie CreditAttribution: daffie commentedBack to RTBC.
Comment #39
xjmCommitted to 8.9.x. Thanks!