I've searched, but although we discussed this in countless of issues already, it seems that no one created a patch for it yet.
In short: required = TRUE must only be set for modules that are absolutely required for core to work.
Field type modules do not belong into that list.
Next to real world Drupal sites, this patch will also speed up the new testing profile.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | drupal.required.17.patch | 2.72 KB | sun |
| #10 | drupal.required.10.patch | 2.71 KB | sun |
| #3 | drupal.required.3.patch | 2.1 KB | sun |
| #2 | drupal.required.2.patch | 2.68 KB | sun |
| drupal.required.0.patch | 2.64 KB | sun |
Comments
Comment #2
sunThere must be a bug in the required/dependencies handling...? Text module breaks badly, but even when added as dependency for Node... very odd.
Comment #3
sunok, let Text module be required then. But the other field type modules are definitely not.
Comment #4
sunBetter title?
Comment #5
sunComment #6
moshe weitzman commentedSomething about upgrade path gets more complicated if these modules are non-required. Maybe thats no longer an issue in D7.
Comment #7
sunIf I understood recent changes correctly, then it's not an issue if a newly required dependency does exist, it simply gets enabled. Aside from that, CCK will have to fiddle with these modules anyway, as some have been renamed.
Comment #8
sun#3: drupal.required.3.patch queued for re-testing.
Comment #9
yched commentedIf options.module is not required anymore, then taxonomy and list modules need to have a dependency on it.
Arguably, 'options' could be worth keeping as required - it's nature is to provide generic widgets, will likely be used by many field types..
Other than that, making number and list 'not required' sounds reasonable. We should make sure they're enabled in the default profile, though.
Comment #10
sunThanks! Added Options as dependency for Taxonomy and List.
Comment #11
yched commentedThks sun. Looks good.
Comment #12
webchickHm. This is an API change (modules that assume certain field modules exist need to change their dependencies[] lines). But it might be possible to justify this if there were some real numbers behind "Next to real world Drupal sites, this patch will also speed up the new testing profile."
Comment #13
sunJust tested this while writing a very simple/small test for Form API that leverages the testing profile. Results on my local box:
BEFORE: The test run finished in 25 sec.
AFTER: The test run finished in 22 sec.
I suck at maths, but my simple calc says that this therefore shaves off 12%.
Comment #14
yched commentedNote : if this gets in, http://drupal.org/handbook/modules/field will need to be updated accordingly.
Comment #15
sun#10: drupal.required.10.patch queued for re-testing.
Comment #17
sunRe-rolled against HEAD.
Comment #18
webchickI spoke to Dries about this. Even though this represents an API change, it's a pretty low-impact one (a couple of extra dependencies[] lines in the .info file) and if we're going to make it, it needs to be before RC.
In addition to providing more flexibility for installation profiles/distributions to make decisions on what's /actually/ required, it can help us speed up test runs, and I think this makes Drupal's required module list more intuitive to newcomers as well.
Committed to HEAD. Thanks!
Comment #19
yched commentedUpdated http://drupal.org/handbook/modules/field accordingly
Comment #20
rfayI don't think this API change needs to be announced. Contrary opinions?
Comment #21
webchickPerhaps not. You can see that a few core modules did have to change as a result, and I could see this affecting Date module and maybe some other ones that build upon option lists/checkboxes. Up to you, though.