Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
- Text module is marked as
required: TRUE
in its .info.yml file, even though it is not required.
Details
- Text module was marked as required for an unknown reason.
- This causes the module to be installed under any circumstances, even when not needed. (e.g., WebTests using the Testing profile)
Proposed solution
- Replace the required flag for Text module with proper dependencies (if any are missing).
Comment | File | Size | Author |
---|---|---|---|
#36 | interdiff.txt | 846 bytes | sun |
#36 | drupal8.text-required.36.patch | 11.44 KB | sun |
#34 | interdiff.txt | 2.49 KB | sun |
#34 | drupal8.text-required.34.patch | 11.44 KB | sun |
#32 | 2191285-text-required-32.patch | 11.14 KB | andypost |
Comments
Comment #2
andypostIf string field type will get widget and formatter it would be much easy #2150511: [meta] Deduplicate the set of available field types
Comment #3
sunSince the removal of this required flag originally passed in #1934772: Filter module is not required, but is marked as required, I suspect that this change is blocked on that issue.
Let's wait for the other issue to land first.
Comment #4
Berdirdrupal8.text-required.0.patch queued for re-testing.
Comment #6
alexpottComment #7
BerdirI verified that all modules that call text_summary() or create text fields have the dependency on text.module.
Together with #1541298: Remove Node module dependency from Testing profile, this will remove node, filter and text from the list of modules that every web test needs to enable.
Based on a quick test, this decreased the run time of UserSaveTest from 6 to 5 seconds for me, but that wasn't a very scientific test :)
Comment #8
Berdir6: 2191285.6.patch queued for re-testing.
Comment #10
alexpott6: 2191285.6.patch queued for re-testing.
Comment #11
BerdirNice, now that #1541298: Remove Node module dependency from Testing profile has been committed, tests are now actually running without node, text and filter for the very first time, and that's causing fails related to filter.module as well. Shows that the issue to make filter.module should have waited for this, but I think we can fix it here.
Looks like file.module calls _filter_htmlcorrector() directly, so it needs a dependency on that. Trying again, hopefully with better readable results.
Comment #13
xjmThis will need a change record based on #1934772-39: Filter module is not required, but is marked as required.
https://drupal.org/node/1397856 is the change record to look at.
Comment #15
Berdir11: 2191285.11.patch queued for re-testing.
Comment #17
BerdirNice, now we have some real test fails that show that filter.module can actually *not* be an optional module, as there are for example calls to _filter_htmlcorrector() in mail.inc :)
Looking at that, should we move that function to Utility\Xss?
Comment #18
sunThe HTML Corrector filter is not related to XSS, it just fixes malformed HTML (missing closing element tags, etc) by loading the HTML (soup) string into a DOMDocument and saving it back into a string.
Note that #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5 proposes to replace the internal implementation with a PHP library.
Comment #19
longwaveSo is Utility\String a suitable place? The four functions in there all handle HTML already.
Comment #20
sunLooks like this issue revealed a very deeply hidden bug in our module dependencies, which was masked by 'required' modules previously.
→ Flawed assumptions. Flawed architecture. Yet another major reason to #1688162: Replace required flag of modules with proper dependencies
Postponing on #2195745: Replace _filter_htmlcorrector() with a utility class in core
Comment #21
BerdirComment #22
jibran#2195745: Replace _filter_htmlcorrector() with a utility class in core is in.
Comment #23
jibranNous allons ici.
Comment #24
Damien Tournoud CreditAttribution: Damien Tournoud commented11: 2191285.11.patch queued for re-testing.
Comment #27
sunFile module's dependency on Text is gone.
Attached patch should fix the test failures. Not sure whether I got
Module\DependencyTest
right... (I always avoid to run that locally)Comment #29
jibranWhat is the reason?
Comment #30
BerdirThe reason is that it takes forever to run ;)
I'll have a look later today I hope.
Is the dependency for file module still necessary? I only added that due to the call to _filter_htmlcorrector(), before seeing the other calls and we fixed that.
Comment #31
andypostWill be after #1856562: Convert "Subject" and "Message" into Message base fields
Comment #32
andypostFix latest test
PS: it's easy to comment-out passed test* and leave only one that was broken, so it takes
42 sec
for me.Comment #33
BerdirEnabling filter here would also be a valid test fix, not sure if it is worth to check a third file.
Does it really? Weren't those just test fails because they tried to add text fields to contact forms? If possible, I'd prefer to add the dependency to the tests, not contact.module.
Also, based on those noticed, I'm guessing the field_ui is not handling the now possible case that there are no field types properly.
When you are changing the text anyway, add Entity Reference? Instead of opening another issue...
Comment #34
sunThe other two simple (non-entity) config settings files are completely sufficient for that test.
So attached patch addresses 2. + 3. only.
Comment #36
sunI can only hope that this is not a random test failure... let's see.
Comment #37
BerdirYeah, I had a similar problem with the node issue where that list wasn't the same locally and on the testbot. It does seem to be stable on the same environment though. The test doesn't seem to be failing on 5.4/5.5 on testbot. But it is scary, maybe we should switch to a different module with a smaller and more specific dependency tree? We've extended this line multiple times now, always adding more to the list as we change dependencies and make more modules optional...
Not in this issue....
Comment #38
andypostprobably related, suppose needs separate issue but
CommentDefaultFormatter::viewElements()
uses$this->settings['pager_id']
so defaults are skipped, this should be$this->getSetting('pager_id')
Comment #39
sunThis patch appears to decrease the total test suite time by ~2 minutes on one of the fastest testbots (#1883):
Comment #40
BerdirThe fast/physical test bots have a fairly reliable text execution time now unlike the virtual ones, so that might actually mean something yes. Some sort of dependency tree/count of tests that don't rely on text/node and are now avoiding those 3 modules would be interesting, not sure how many of those we really have. Might write something :)
Patch looks great and is quite small now, that's great.
A follow-up to investigate the warnings thrown in field_ui if there are no field_types available would be great but this is RTBC.
Note that this issue doesn't need a new change record, we only need to update the existing one linked in #13, which is trivial and we don't need to prepare anything for that, so we're good.
Comment #41
catchCommitted/pushed to 8.x, thanks!
Seems like we ought to be close to Field module not being required now?
Comment #42
sunThat's an interesting question. I've created
#2199637: Replace "required" flag of Field module with proper dependencies
and also updated the issue summary of #1688162: Replace required flag of modules with proper dependencies to give a more holistic overview of the remaining 'required' modules.
Comment #43
BerdirUpdated https://drupal.org/node/1397856, now mentions all modules that were made optional so far.
Comment #44
xjmLooks like the change record is taken care of. Thanks!