Problem/Motivation

The migration UI re-uses the installation database form to gather db credentials from the user. This works great apart from the fact that we do:

    $db_prefix = ($profile == 'standard') ? 'drupal_' : $profile . '_';
    $form['advanced_options']['prefix'] = [
      '#type' => 'textfield',
      '#title' => t('Table name prefix'),
      '#default_value' => empty($database['prefix']) ? '' : $database['prefix'],
      '#size' => 45,
      '#description' => t('If more than one application will be sharing this database, a unique table name prefix – such as %prefix – will prevent collisions.', ['%prefix' => $db_prefix]),
      '#weight' => 10,
    ];

The description makes no sense in the migrate case (it does during installation).

This was found in #3086374: Make Drupal 8 & 9 compatible with PHP 7.4 due to the global $install_state not being set and new errors being emitted from PHP. Running Drupal\Tests\migrate_drupal_ui\Functional\d6\MultilingualReviewPageTest showed the error.

Proposed resolution

Remove the description.
Before

After

Original proposal
We should change the description for this form field in \Drupal\migrate_drupal_ui\Form\CredentialForm::buildForm() and perhaps rework the logic in \Drupal\Core\Database\Install\Tasks::getFormOptions so we don't set a nonsense one for migrate in the first place.

Remaining tasks

User interface changes

New UI text on migrate credentials form.

API changes

None

Data model changes

None

Release notes snippet

CommentFileSizeAuthor
#10 interdiff-6-9.txt886 bytesjungle
#10 3110839-9.patch868 bytesjungle
#6 After-mysql.png32.24 KBquietone
#6 Before-mysql.png27.3 KBquietone
#6 3110839-6.patch868 bytesquietone

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Related issues: +#3086374: Make Drupal 8 & 9 compatible with PHP 7.4
amjad1233’s picture

amjad1233
MJED
Location Brisbane
commented
Assigned: Unassigned » amjad1233

@alexpott Do you have anything in mind for the description of the field?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Well it needs to tell the user to set it to the prefix used in the drupal site you are migrating from.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

quietone commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3110839-6.patch868 bytes
new Before-mysql.png27.3 KB
new After-mysql.png32.24 KB

I'm not sure I agree with alexpott and the original proposed resolution because the title for the section is 'Provide credentials for the database of the Drupal site you want to upgrade.' and I don't see the need to repeat that. With that in mind, here is a patch that removes the description.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Assigned: amjad1233 » Unassigned
Status: Needs review » Reviewed & tested by the community

I agree with @quietone, we don't need a description for the field, the form description describes what to do. I don't think we need a test for this 'bug report' as we are just making some UI text changes.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Bug Smash Initiative 
+++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
@@ -134,6 +134,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      unset($form["database"]["settings"][$key]["advanced_options"]["prefix"]['#description']);

super uber nit pick - any reason we're mixing single and double quotes here? single will suffice in my book

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Needs review » Needs work

nw for #8

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 3110839-9.patch868 bytes
new interdiff-6-9.txt886 bytes

Replaced all double quotes with single ones where possible.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented

Yes, +1 for back to RTBC

  • larowlan committed 7aca346 on 9.1.x 
    Issue #3110839 by quietone, jungle: Use of \Drupal\Core\Database\Install...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Title: Use of \Drupal\Core\Database\Install\Tasks::getFormOptions() in \Drupal\migrate_drupal_ui\Form\CredentialForm::buildForm() results in confusing description for prefix form element » [backport] Use of \Drupal\Core\Database\Install\Tasks::getFormOptions() in \Drupal\migrate_drupal_ui\Form\CredentialForm::buildForm() results in confusing description for prefix form element
Version: 9.1.x-dev » 9.0.x-dev
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Committed 7aca346 and pushed to 9.1.x. Thanks!

  • larowlan committed 67c0d6b on 8.9.x 
    Issue #3110839 by quietone, jungle: Use of \Drupal\Core\Database\Install...

  • larowlan committed 769509b on 9.0.x 
    Issue #3110839 by quietone, jungle: Use of \Drupal\Core\Database\Install...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Title: [backport] Use of \Drupal\Core\Database\Install\Tasks::getFormOptions() in \Drupal\migrate_drupal_ui\Form\CredentialForm::buildForm() results in confusing description for prefix form element » Use of \Drupal\Core\Database\Install\Tasks::getFormOptions() in \Drupal\migrate_drupal_ui\Form\CredentialForm::buildForm() results in confusing description for prefix form element
Version: 9.0.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Backported to 9.0.x and 8.9.x as this is a bug and there is little risk of disruption

Status: Fixed » Closed (fixed)

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