Problem/Motivation

Many moons ago we made the field module not required. This subtlety broke an assumption made by \Drupal\Core\Installer\Form\SiteConfigureForm. It does
$this->moduleInstaller->install(['file', 'update'], FALSE); under the impression that all of these modules dependencies are required. This is no longer true.

So if you install the testing profile via Drush core.extension:module looks like:

module:
 dynamic_page_cache: 0
 file: 0
 page_cache: 0
 system: 0
 update: 0
 user: 0
 testing: 1000

Proposed resolution

Add field to the list of modules to install.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#22 3033494-20.8_6_x.patch2.1 KBalexpott
#20 3033494-20.8_7_x.patch67.68 KBdww
#20 3033494-20.8_7_x.test-only.patch1.25 KBdww
#20 3033494.16_20.8_6_x.interdiff.txt921 bytesdww
#20 3033494-20.8_6_x.patch2.1 KBdww
#20 3033494-20.8_6_x.test-only.patch1.25 KBdww
#16 3033494-16.patch2.14 KBalexpott
#16 3033494-16.8-7-x.patch67.72 KBalexpott
#7 3033494-3.patch2.17 KBalexpott
#7 3033494-5.patch67.75 KBalexpott
#5 3033494-5.patch67.75 KBalexpott
#5 3-5-interdiff.txt65.58 KBalexpott
#3 3033494-3.patch2.17 KBalexpott
#3 2-3-interdiff.txt1.18 KBalexpott
#2 3033494-2.patch2.15 KBalexpott
#2 3033494-2-test-only.patch1.26 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Active » Needs review
FileSize
3033494-2-test-only.patch1.26 KB
3033494-2.patch2.15 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
2-3-interdiff.txt1.18 KB
3033494-3.patch2.17 KB

Fixing some code comments.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Needs work

We need to fix the DB dump introduced in #3020718: system_post_update_add_expand_all_items_key_in_system_menu_block fails if blocks module is disabled.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
3-5-interdiff.txt65.58 KB
3033494-5.patch67.75 KB

Sure - it's fixed in #2863652: Do not allow files with "php|pl|py|cgi|asp|js" extensions to be renamed to *.txt and be uploaded if *.txt is not allowed by the field widget because that's where this breaks but we can re-roll the one that get's in last.

Status: Needs review » Needs work

The last submitted patch, 5: 3033494-5.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
3033494-5.patch67.75 KB
3033494-3.patch2.17 KB

This comment uploads the patch for 8.7.x and the one for 8.6.x as well.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +DrupalWTF

Thanks, for addressing the feedback. Code changes and test looks good so marking it RTBC.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

Speaking of DrupalWTF, why does update.module care about file.module or field.module? ;) I don't remember writing any such code years ago when I added update.module to core. I'm not sure this patch is moving in the right direction. Seems we should only enable update.module if that's what they want. What's this "non-required dependencies" business mentioned in the comment for testUpdateModuleInstall()?

Okay, a little more grepping, and I see ./src/Form/UpdateManagerInstall.php is calling file_save_upload() which indeed lives in file.module. Weird. Maybe that's my fault, I don't remember. ;) But, that doesn't require a file field, right?

Is this not "works as designed"?

Seems the better fix is to have a less weird file API so that update module can have a file upload form element that doesn't require file.module itself?

Tentatively -1 to this patch.

alexpott’s picture

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

@dww back in the day filed module was required so you didn’t have to care. Personally I think we should do the quick fix here and resolve this differently in a follow up if we want to. The case where field module is not installed is extremely rare. Both minimal and standard install it.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

But then why is this a bug? If a site is truly bare-bones, and is only installing file.module to help update.module, then what's the harm in having only file.module enabled without field.module?

I don't mean to hold up progress, I'm just trying to understand.

Sorry/thanks,
-Derek

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented 
+++ b/core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php
@@ -292,7 +292,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
-      $this->moduleInstaller->install(['file', 'update'], FALSE);
+      $this->moduleInstaller->install(['field', 'file', 'update'], FALSE);

Question along similar lines. Why is this not $this->moduleInstaller->install(['update']); and rely on the default value of $enable_dependencies?
If there is a really good reason to bypass that, I think it deserves a comment.

(The new test added by this issue passes with the above change, but not uploading a new patch in case that derails this)

alexpott’s picture

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

@dww The file module is dependent on the field module. That's the way it is. It we want to disentangle that. Then so be it. But I think that that should happen in a followup. This bug only occurred because someone when on a mission to try to remove all required modules are field was one they were successful with.

@tim.plunkett I've no idea why we call that with FALSE for dependencies. I agree with you it's wrong. The system at this point is fully working - I'll have a look in git history and see if I can work out why.

catch’s picture

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

Agreed with #12, let's either make that change or add a comment explaining why not.

alexpott’s picture

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

The patch that added this was the original ability to install modules without checking dependencies - #399642-92: Replace drupal_install_modules() with an improved module_enable() (note the issue is confusing because there are followup commits on it). At this point update had no dependencies. It should have because it was using file_save_upload(). That was fixed in #1468328: Move file entity info, managed file, and file usage functionality into File module by adding the file module to this and this also added the missing dependency.

So as far as I can work it out the only possible reasons we used the no dependency module install is (a) at the time update has no dependencies (b) to copy the other early module installs done in the installer.

(a) is no longer true and (b) is irrelevant because the system is fully installed at this point.

So let's do #12. And then if we every untangle update / file / field then this will do the right thing automatically.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
3033494-16.8-7-x.patch67.72 KB
3033494-16.patch2.14 KB
alexpott’s picture

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

With the patch in #16 I've manually tested drush install of minimal and the testing profile. It works and here's the enabled modules after an install of the testing profile.

Package Name Type Version
Core Field (field) Module 8.7.0-dev
Core Internal Dynamic Page Cache (dynamic_page_cache) Module 8.7.0-dev
Core Internal Page Cache (page_cache) Module 8.7.0-dev
Core System (system) Module 8.7.0-dev
Core Update Manager (update) Module 8.7.0-dev
Core User (user) Module 8.7.0-dev
Field types File (file) Module 8.7.0-dev
Core Classy (classy) Theme 8.7.0-dev

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs review » Reviewed & tested by the community

Great, I'm glad this worked! It is much clearer and much less brittle.

alexpott’s picture

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

@tim.plunkett yep totally. Thanks for spotting this, I don't know why I just continued adding modules.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Reviewed & tested by the community » Needs review
FileSize
3033494-20.8_6_x.test-only.patch1.25 KB
3033494-20.8_6_x.patch2.1 KB
3033494.16_20.8_6_x.interdiff.txt921 bytes
3033494-20.8_7_x.test-only.patch1.25 KB
3033494-20.8_7_x.patch67.68 KB

Okay, fair enough. I definitely agree that untangling all the file upload API stuff is out of scope for the bug at hand.

I still don't like this comment/assertion:

+++ b/core/tests/Drupal/FunctionalTests/Installer/TestingProfileInstallTest.php
@@ -0,0 +1,38 @@
+      'The Update module and its non-required dependencies are installed.'

WTF is a "non-required dependency"? ;) Oxymoron alert.
s/non-required//
(both in the assertion and in the comment).

New set of patches for both branches.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs review » Reviewed & tested by the community

@dww indeed that's totally an oxymoron - thanks for fixing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
3033494-20.8_6_x.patch2.1 KB

Re-upping the 8.6.x patch from #20 so rtbc retester doesn't come along and set this to needs work.

The last submitted patch, 20: 3033494-20.8_6_x.test-only.patch, failed testing. View results

The last submitted patch, 20: 3033494-20.8_7_x.test-only.patch, failed testing. View results

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 3033494-20.8_6_x.patch, failed testing. View results

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs work » Reviewed & tested by the community

#3035320: Random fails in TimestampAgoFormatterTest and DateTimeTimeAgoFormatterTest strikes again. :/

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 3033494-20.8_6_x.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Reviewed & tested by the community

  • larowlan committed 5d2bc24 on 8.7.x 
    Issue #3033494 by alexpott, dww, tim.plunkett: SiteConfigureForm can...

  • larowlan committed 6869324 on 8.6.x 
    Issue #3033494 by alexpott, dww, tim.plunkett: SiteConfigureForm can...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed 5d2bc24 and pushed to 8.7.x. Thanks!

c/p 6f3f6fb3ba as and pushed to 8.6.x

This commit was brought to you by 'Qualified' by 'Dr John' 🎶

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

I think we should have committed https://www.drupal.org/files/issues/2019-02-19/3033494-20.8_7_x.patch to 8.7.x but we can do that in #2863652: Do not allow files with "php|pl|py|cgi|asp|js" extensions to be renamed to *.txt and be uploaded if *.txt is not allowed by the field widget now.

  • larowlan committed 56606fe on 8.7.x 
    Revert "Issue #3033494 by alexpott, dww, tim.plunkett: SiteConfigureForm...

  • larowlan committed 3b32b1e on 8.7.x 
    Issue #3033494 by alexpott, dww, tim.plunkett, larowlan:...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

@alexpott let me know there was an 8.7 specific patch here so reverted the previous commit and applied and pushed that as 3b32b1e

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

FWIW, we updated the DB dump after my feedback in #4. :)

alexpott’s picture

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

I ticked the credit box for @jibran for #4.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

Thanks!

Status: Fixed » Closed (fixed)

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