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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

Something like this.

quietone’s picture

Status: Active » Needs review
quietone’s picture

Title: Change text from Drupal 8 to Drupal 9 » Use \Drupal::VERSION to get version, not hard code it
daffie’s picture

Status: Needs review » Needs work
</code><code>
+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeFormBase.php
@@ -60,6 +67,8 @@ public static function create(ContainerInterface $container) {
+    list($this->destinationSiteVersion) = explode('.', \Drupal::VERSION, 2);

+++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
@@ -107,6 +107,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    list($destination_site_version) = explode('.', \Drupal::VERSION, 2);

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php
@@ -19,6 +26,9 @@ protected function setUp() {
+    [$this->destinationSiteVersion] = explode('.', \Drupal::VERSION, 2);

@@ -34,7 +44,9 @@ public function testMigrateUpgradeExecute() {
+    [$this->destinationSiteVersion] = explode('.', \Drupal::VERSION, 2);

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeFormStepsTest.php
@@ -62,7 +62,9 @@ public function testMigrateUpgradeReviewPage() {
+    list($destination_site_version) = explode('.', \Drupal::VERSION, 2);

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/IdConflictTest.php
@@ -74,7 +74,9 @@ public function testMigrateUpgradeExecute() {
+    list($destination_site_version) = explode('.', \Drupal::VERSION, 2);

We 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()

quietone’s picture

Status: Needs work » Needs review
FileSize
3.46 KB
16.92 KB

I 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.

Status: Needs review » Needs work

The last submitted patch, 6: 3133305-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
16.92 KB

The test failure is unrelated but there were coding standard errors.

longwave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs followup

I 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.

quietone’s picture

Issue tags: -Needs followup

Follow up made #3134417: Add \Drupal::getMajorVersion() and queued a test for 9.0.x

benjifisher’s picture

Will this issue need an update if #3024682: Migrate UI - add human-friendly module names to the Review form is fixed first?

xjm’s picture

Title: Use \Drupal::VERSION to get version, not hard code it » Use \Drupal::VERSION to get the version in Migrate UI rather than hardcoding it
xjm’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php
@@ -34,7 +44,8 @@ public function testMigrateUpgradeExecute() {
+    // Get the current major version.
+    $session->responseContains("Upgrade a site by importing its files and the data from its database into a clean and empty new install of Drupal $this->destinationSiteVersion.");

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/IdConflictTest.php
@@ -74,7 +74,8 @@ public function testMigrateUpgradeExecute() {
+    // Get the current major version.
+    $session->responseContains("Upgrade a site by importing its files and the data from its database into a clean and empty new install of Drupal $this->destinationSiteVersion.");

This 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.

benjifisher’s picture

I hesitated a bit about the explodes and array magic because my first thought was "there must be API for this".

I 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:

+  /**
+   * The '8.x-' prefix is used on contrib module version numbers.
+   *
+   * @var string
+   */
+  const CORE_PREFIX = '8.x-';
...
+  public static function createFromVersionString($version_string) {
... (remove the prefix if it is there)
+    $version_parts = explode('.', $version_string);
+    $major_version = $version_parts[0];
...
daffie’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php
@@ -34,7 +44,8 @@ public function testMigrateUpgradeExecute() {
+    // Get the current major version.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/IdConflictTest.php
@@ -74,7 +74,8 @@ public function testMigrateUpgradeExecute() {
+    // Get the current major version.

These 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.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

I 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.

Yeah me too! I read the whole semver namespace there and TBH it's kind of a mess. 😲But nothing to help us there.

These 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.

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!

quietone’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
16.84 KB

#15 Fixes. Comments removed.

@daffie, why should these comments be removed?

daffie’s picture

Status: Needs review » Needs work

Hi @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.

quietone’s picture

Status: Needs work » Needs review
FileSize
1.7 KB
16.84 KB

@daffie, thanks.

The patch in #17 is bad, just ignore it. I started again from the patch in #8

daffie’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @quietone for the new patch. See my apology in comment #18.
Back to RTBC.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/migrate_drupal_ui/src/Form/IdConflictForm.php
@@ -94,7 +94,10 @@ protected function conflictsForm(array &$form, array $conflicts) {
+      '#markup' => '<p>' . $this->t('It looks like you have content on your new site which <strong>may be overwritten</strong> if you continue to run this upgrade. The upgrade should be performed on a clean Drupal :version installation. For more information see the <a target="_blank" href=":id-conflicts-handbook">upgrade handbook</a>.', [
+        ':version' => $this->destinationSiteVersion,

+++ b/core/modules/migrate_drupal_ui/src/Form/IncrementalForm.php
@@ -82,7 +82,10 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      '#prefix' => $this->t('An upgrade has already been performed on this site. To perform a new migration, create a clean and empty new install of Drupal :version. Rollbacks are not yet supported through the user interface. For more information, see the <a href=":url">upgrading handbook</a>.', [
+        ':version' => $this->destinationSiteVersion,

+++ b/core/modules/migrate_drupal_ui/src/Form/OverviewForm.php
@@ -34,7 +34,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      '#markup' => '<p>' . $this->t('Upgrade a site by importing its files and the data from its database into a clean and empty new install of Drupal :version. See the <a href=":url">Drupal site upgrades handbook</a> for more information.', [
+          ':version' => $this->destinationSiteVersion,

@@ -45,7 +46,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    $form['legend']['#markup'] .= '<dd>' . $this->t('This empty Drupal :version installation you will import the old site to.', [':version' => $this->destinationSiteVersion]) . '</dd>';

Sorry that I didn't notice this before, but the version should use a plain @placeholder rather than a :url_placeholder.

Sorry!

quietone’s picture

Status: Needs work » Needs review
FileSize
4.74 KB
16.84 KB

@xjm, that's OK. I could use the incentive to learn how to use those placeholders correctly.

All points in #21 fixed.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

The 4 placeholders are changed from ":placeholder" to "@placeholder".
Back to RTBC.

  • xjm committed 4d49b84 on 9.1.x
    Issue #3133305 by quietone, daffie, xjm, benjifisher, longwave: Use \...

  • xjm committed 7dabc93 on 8.9.x
    Issue #3133305 by quietone, daffie, xjm, benjifisher, longwave: Use \...
xjm’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 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!

  • xjm committed f638e91 on 9.0.x
    Issue #3133305 by quietone, daffie, xjm, benjifisher, longwave: Use \...

  • xjm committed 6704a2f on 9.0.x
    Revert "Issue #3133305 by quietone, daffie, xjm, benjifisher, longwave:...
xjm’s picture

Status: Fixed » Patch (to be ported)
+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php
@@ -19,6 +26,9 @@ protected function setUp() {
+    [$this->destinationSiteVersion] = explode('.', \Drupal::VERSION, 2);

Whoopsidaisy, 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.

  • xjm committed 0902d6a on 9.0.x
    Revert "Revert "Revert "Issue #3133305 by quietone, daffie, xjm,...
  • xjm committed 868ae52 on 9.0.x
    Revert "Revert "Issue #3133305 by quietone, daffie, xjm, benjifisher,...

  • xjm committed 5a4fef2 on 8.9.x
    Revert "Issue #3133305 by quietone, daffie, xjm, benjifisher, longwave:...

  • xjm committed 9a2385e on 9.0.x
    Issue #3133305 by quietone, xjm, daffie, benjifisher, longwave: Use \...
xjm’s picture

...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?)

quietone’s picture

Status: Patch (to be ported) » Needs review
FileSize
979 bytes
17.13 KB

And now a patch for 8.9.x.

daffie’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php
@@ -28,12 +28,13 @@ protected function setUp() {
   /**
    * Executes all steps of migrations upgrade.
    *
+   *
    * The upgrade is started three times. The first time is to test that
    * providing incorrect database credentials fails as expected. The second
    * time is to run the migration and assert the results. The third time is

Nitpick: Can we undo this change.

quietone’s picture

Status: Needs work » Needs review
FileSize
748 bytes
16.85 KB

Oh, that is silly mistake.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

  • xjm committed bed0776 on 8.9.x
    Issue #3133305 by quietone, xjm, daffie, benjifisher, longwave: Use \...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.9.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.