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.
Part of #1688162: Replace required flag of modules with proper dependencies
Objective
- After #2191285: Text module is not required, but is marked as required, no field type module is marked as "required" anymore.
- As a consequence, Field module itself should theoretically no longer required now.
Proposed solution
- Remove the
required
flag from Field module and declare proper dependencies instead.
Comment | File | Size | Author |
---|---|---|---|
#42 | optional-field-module-2199637-42.patch | 21.42 KB | swentel |
#37 | optional-field-module-2199637-37-interdiff.txt | 1.54 KB | Berdir |
#37 | optional-field-module-2199637-37.patch | 21.41 KB | Berdir |
#35 | optional-field-module-2199637-35.patch | 21.39 KB | Berdir |
#30 | optional-field-module-2199637-30-interdiff.txt | 4.92 KB | Berdir |
Comments
Comment #1
sunJust to see what happens.
Probably nothing (passing tests), due to dependencies of dependencies.
Comment #3
sunInteresting. User depends on Field.
Comment #4
andypost@sun user picture is a field, so probably image also needed
Comment #5
sunThe dependency graph in #1688162-10: Replace required flag of modules with proper dependencies explains why the patch passed after making User module depend on Field module → User module is required, so Field module is always installed.
I wonder whether it is possible to make the dependency on the 'field.info' service optional?
Comment #6
andypostFieldInfo is needed for
FieldableDatabaseStorageController
and only forfieldInfo->getBundleInstances()
So at least any fieldable entity require it to properly populate fields, probably related #2114707: Allow per-bundle overrides of field definitions
Comment #7
yched CreditAttribution: yched commentedThe goal is to replace field.module's FieldInfo with a registry of base + configurable fields handled by Core/Field in #2116363: Unified repository of field definitions (cache + API).
Comment #8
sunHm, yeah, while it would be simple to make the injected FieldInfo service optional, the actual crux is this already:
So unless we're able to make FieldInfo also optional for
FieldableDatabaseStorageController
, I can only guess this means that this issue must be postponed on #2114707: Allow per-bundle overrides of field definitions and #2116363: Unified repository of field definitions (cache + API) — and after those, this issue might be obsolete, but let's see...Comment #9
sunI found a solution :-)
Installer says: "Installing 4 of 4 modules." :)
The trick was to supply a
Drupal\Core\Field\FieldInfoNull
implementation that just returns no field info for all methods + register that in core.services.yml, so that it is used, unless Field module is installed.Unfortunately, some calls to procedural field module functions appear to be scattered across core, so even though I'm able to successfully finish the installation, visiting the front page (/user/1) blows up, just because:
:-/
Comment #11
sunOh hey, some more quick adjustments + I'm able to view /user/1 and /user/1/edit works, too :-)
Comment #13
sunFixed
FieldInfo
is incompatible withFieldInfoInterface
.→ I had to remove the type-hints for some Field module classes, because
Drupal\Core\Field\FieldInfoInterface
can obviously not type hint those. ;)Obviously, I just want to see how far we can get with this approach... so please don't take this too seriously :)
Comment #15
BerdirThis will be a lot easier after #2167167: Remove field_info_*() ;)
Comment #16
BerdirLet's see what happens. A lot of field type modules like text already depend on field, so most dependencies should already be covered...
Comment #18
BerdirStupid DependencyTest is stupid! ;)
Comment #20
sunYes, field modules are specifying a dependency on Field module already. However, what about the opposite direction?
Doesn't User module still depend on Field module?
(User module is installed separately as the second module right after System module by the installer.)
In essence, the question is whether you can actually install Drupal without Field module. This can be tested by hitting the installer with
install.php?langcode=en&profile=testing
Comment #21
BerdirNo, user does not depend on field because the field.info service is gone. The integration of configurable fields into entity storage is 99% optional, field.module hooks into the relevant processes.
There are still a handful of references/dependencies on field.module in Drupal\Core\Field|Entity, see #2079427: Core/Entity depends on classes / functions from field.module, but they're going away, nothing blocking left.
There is one exception and I hit that when I tried to move the user roles table to a normal field table that is managed by the entity storage, see #2248977: Complete support for multi-value base fields in ContentEntitySchemaHandler and use it for the user.roles field. But that's just a stupid static method call that is already fixed in @plach's entity storage issue, see the issue above.
Comment #22
BerdirTesting by removing the required property + "drush si -y testing", works fine.
Comment #23
martin107 CreditAttribution: martin107 commentedThis makes perfect sense to me, but before RTBC lets see what testbot thinks there have been lots of commits.
today, locally I too can drush site install :)
Comment #26
BerdirInteresting, now we actually had test fails :)
Fixed two problems:
- WidgetBase called functions from field.module for that filter xss stuff. Moved those to WidgetBase for now as public static, they're also used from some formatters and views. Not sure if there's a better place for this? a helper class maybe? With this change, the contact tests, which are now using widgets, are verifying that you can do this for base fields and core field types, without field.module.
- content_translation_install() is updating field_storage_config entities, added a module exists check there as it would theoretically be possible to use it without field.module.
Comment #28
BerdirComment #30
BerdirForgot to update the calls to that method. Pretty sure that WidgeBase is not the right place now :)
Comment #32
yched CreditAttribution: yched commentedOuch - yeah, not sure where to put field_filter_xss_*(), and WidgetBase is not the right place. A FieldXss utility class ? :-/
As a side note, I didn't closely follow the autoescape / SafeMarkup thing, but the resulting code in field_filter_xss() is a bit weird :
#1825952: Turn on twig autoescape by default did :
but Xss::filter() already calls SafeMarkup::set(), so we call it twice ?
Backtracking through history:
- D7 just does
return filter_xss($string, _field_filter_xss_allowed_tags())
there.- #1798654: Clean faulty HTML in help description of field widgets wrapped that in _filter_htmlcorrector()
- #2195745: Replace _filter_htmlcorrector() with a utility class in core replaced _filter_htmlcorrector(filter_xss()) with Html::normalize(Xss::filter())
- #1825952: Turn on twig autoescape by default wrapped the above in SafeMarkup::set(), but also made Xss::filter() itself call SafeMarkup::set()...
Pinging the folks in #1825952: Turn on twig autoescape by default.
Comment #33
andypostRelated #2309929: HTML double-escaping in field forms
Comment #34
yched CreditAttribution: yched commentedFrom @chx :
"You are feeding a safe string to Html::normalize so the result will be safe too but the output is not necessarily the same as the input so the new string needs to be marked as safe too"
Then, maybe we could Html::normalize() *before* passing to Xss::filter() ?
Pinging the folks in #1798654: Clean faulty HTML in help description of field widgets.
Comment #35
BerdirHm, what about making it a fancy trait? Has the limitation that only OO code can use it, but all calls in HEAD are already in classes.
Did not change anything about those calls, not sure how related it is for this issue, as I'm only moving it.
Comment #36
swentel CreditAttribution: swentel commentedTrait works for me. Mostly nitpicks left.
Maybe this shouldn't live in Core/Field but somewhere more general, like in Component\Utility ?
80 chars
same
redundant space
Comment #37
BerdirImproved documentation. We agreed that we keep this trait in Core\Field for now.
Comment #38
swentel CreditAttribution: swentel commentedLooks good to me. I was confused first about the removal of the static on sanitizeLabel() but it would now work anymore otherwise.
Comment #39
alexpottWe need a change record because we're removing functions and introducing a trait.
Comment #40
swentel CreditAttribution: swentel commentedChange record at https://www.drupal.org/node/2337599
Tentatively setting to RTBC again.
Comment #41
alexpottNeeds a reroll
Comment #42
swentel CreditAttribution: swentel commentedComment #43
alexpottCommitted 5de9e40 and pushed to 8.0.x. Thanks!
Cleaned up the use statements on commit.