Problem/Motivation

SiteSettingsForm calls functions like drupal_get_database_types() and drupal_install_config_directories(), which are defined in core/includes/install.inc, but that file is not included. It can get away with this on Drupal installation because it's called from install.core.inc, which does include install.inc, but when reusing the form class in another context (specifically, for the database credentials step of the upgrade form for migration) it fails. Attached patch includes install.inc in each method referencing functions defined there - alternatively, we could just include it at the file level...

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#45 2272011-45.patch3.98 KBquietone
#45 interdiff-40-45.txt1.61 KBquietone
#40 2272011-40.patch5.71 KBquietone
#40 2272011-40-fail.patch4.19 KBquietone
#40 diff-23-40.txt5.93 KBquietone
#23 sitesettingsform_needs-2272011-23.patch4.26 KBmashermike
#18 sitesettingsform_needs-2272011-18.test_only.patch2.73 KBmashermike
#18 sitesettingsform_needs-2272011-18.patch4.26 KBmashermike
#13 sitesettingsform_needs-2272011-13.patch4.38 KBNitesh Sethia
#11 sitesettingsform_needs-2272011-11.patch4.26 KBhitesh-jain
#7 2272011-7.patch4.26 KBrpayanm
#4 install-form-includes-2272011-4.patch4.09 KBmashermike
#4 install-form-includes-2272011.test_only.patch2.64 KBmashermike
#1 install-form-includes-2272011-1.patch1.45 KBmikeryan
install-form-includes.patch1.45 KBmikeryan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan commented
FileSize
install-form-includes-2272011-1.patch1.45 KB

Proceeding a bit farther with testing, validateForm() actually needs to include install.core.inc since that's where install_database_errors() is defined.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan commented
mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan commented

1: install-form-includes-2272011-1.patch queued for re-testing.

mashermike’s picture

mashermike CreditAttribution: mashermike commented
FileSize
install-form-includes-2272011.test_only.patch2.64 KB
install-form-includes-2272011-4.patch4.09 KB

Added a test only patch and a combination of the fix and tests.

The last submitted patch, 4: install-form-includes-2272011.test_only.patch, failed testing.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm commented
Status: Needs work » Needs review
FileSize
2272011-7.patch4.26 KB
rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm commented
Issue tags: -Needs reroll
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Sorry this needs another reroll :(

I'll try to review sooner next time.

hitesh-jain’s picture

hitesh-jain CreditAttribution: hitesh-jain at Acquia commented
Assigned: Unassigned » hitesh-jain
hitesh-jain’s picture

hitesh-jain CreditAttribution: hitesh-jain at Acquia commented
Assigned: hitesh-jain » Unassigned
Status: Needs work » Needs review
FileSize
sitesettingsform_needs-2272011-11.patch4.26 KB

Rerolled Patch #7 to Latest Code base . Thanks !

Status: Needs review » Needs work

The last submitted patch, 11: sitesettingsform_needs-2272011-11.patch, failed testing.

Nitesh Sethia’s picture

Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commented
Status: Needs work » Needs review
FileSize
sitesettingsform_needs-2272011-13.patch4.38 KB

Rerolled the patch as per the latest release of D8.

Status: Needs review » Needs work

The last submitted patch, 13: sitesettingsform_needs-2272011-13.patch, failed testing.

opdavies’s picture

opdavies
He/him
Primary language English
Location Wales
CreditAttribution: opdavies as a volunteer commented
Issue tags: -Needs reroll

The patch in #13 applies cleanly to HEAD.

Status: Needs work » Needs review

Nitesh Sethia queued 13: sitesettingsform_needs-2272011-13.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 13: sitesettingsform_needs-2272011-13.patch, failed testing.

mashermike’s picture

mashermike CreditAttribution: mashermike at Genuine commented
Status: Needs work » Needs review
FileSize
sitesettingsform_needs-2272011-18.patch4.26 KB
sitesettingsform_needs-2272011-18.test_only.patch2.73 KB

I rerolled my original patches and included a new test only version again.

Status: Needs review » Needs work

The last submitted patch, 18: sitesettingsform_needs-2272011-18.test_only.patch, failed testing.

mashermike’s picture

mashermike CreditAttribution: mashermike at Genuine commented
Status: Needs work » Needs review
Issue tags: +The test only patch failed as expected, +so this patch is ready for review.
mashermike’s picture

mashermike CreditAttribution: mashermike at Genuine commented
Issue tags: -The test only patch failed as expected, -so this patch is ready for review.

The test only patch failed as expected, so this patch is ready for review.

Status: Needs review » Needs work

The last submitted patch, 18: sitesettingsform_needs-2272011-18.test_only.patch, failed testing.

mashermike’s picture

mashermike CreditAttribution: mashermike at Genuine commented
Status: Needs work » Needs review
FileSize
sitesettingsform_needs-2272011-23.patch4.26 KB

Ok, i think having the test only patch on the same comment is causing this to be un-reviewable. I am reattaching the patch from #18. It is the fix and the test.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

Retesting the patch (#23) and the fail patch (#18) on 8.2.x.

  1. +++ b/core/modules/system/src/Tests/Installer/SiteSettingsFormTest.php
@@ -0,0 +1,44 @@
+/**
+ * @file
+ * Contains \Drupal\system\Tests\Installer\SiteSettingsFormTest.
+ */
+

  2. +++ b/core/modules/system/tests/modules/install_form_test/src/Form/ExtendedForm.php
@@ -0,0 +1,13 @@
+/**
+ * @file
+ * Contains \Drupal\install_form_test\Form\ExtendedForm.
+ */
+

Pretty sure the @file statements can be removed. See the final paragraph in the documentation section, @file: Documenting files.

The last submitted patch, 18: sitesettingsform_needs-2272011-18.test_only.patch, failed testing.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Needs work

Tests are passing on 8.2.x and the fail patch is fail, so all good. Setting to NR for the minor point in #26.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.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.

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

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan as a volunteer and at PreviousNext commented
Status: Needs work » Postponed (maintainer needs more info)

Is this still an issue?

install_begin_request makes sure to load that file

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Version: 9.5.x-dev » 10.1.x-dev
Issue summary: View changes
Status: Postponed (maintainer needs more info) » Needs review
FileSize
diff-23-40.txt5.93 KB
2272011-40-fail.patch4.19 KB
2272011-40.patch5.71 KB

I was curious so I updated the patch and made a fail patch.

The last submitted patch, 40: 2272011-40-fail.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

I was looking at #3256642: Introduce database driver extensions and autoload database drivers' dependencies and it is also adding install.inc to a form. In that case to the migrate Credential form. This may fix that?

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Issue summary: View changes
Related issues: +#3256642: Introduce database driver extensions and autoload database drivers' dependencies
daffie’s picture

daffie CreditAttribution: daffie at Finalist commented
Status: Needs review » Needs work

It looks good to me.
Can we get a patch without the changes to drupalci.yml?
After that it is RTBC for me.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative
FileSize
interdiff-40-45.txt1.61 KB
2272011-45.patch3.98 KB

Thanks.

This patch restores drupalci.yml

Adding tag because this was a bugsmash daily target.

daffie’s picture

daffie CreditAttribution: daffie at Finalist commented
Status: Needs review » Reviewed & tested by the community

All code changes look good to me.
For me it is RTBC.

  • catch committed a56ce007 on 10.1.x 
    Issue #2272011 by mashermike, quietone, mikeryan, rpayanm:...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed a56ce00 and pushed to 10.1.x. Thanks!

Status: Fixed » Closed (fixed)

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