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/Motivation
hook_entity_base_field_info_alter()
(and similarly,hook_entity_bundle_field_info_alter()
) is documented as receiving an array ofFieldDefinitionInterface
(FDI) objects as the first parameter.- The purpose of the hook is to alter those definitions, but by design, FDI doesn't include setters.
- If implementations assume the objects are implemented by core's
BaseFieldDefinition
, and invoke those setters, as the example does, then that will break whenever the module that defines the field changes that implementation decision.
Proposed resolution
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Create a patch | Instructions | ||
Update the issue summary noting if allowed during the beta | Instructions | ||
Draft a change record for the API changes | Instructions |
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#46 | 2346329-46.patch | 10.5 KB | SpadXIII |
#44 | 2346329-44.patch | 13.74 KB | Nikhil_110 |
#39 | 2346329-39.patch | 11.13 KB | karishmaamin |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedI don't know how much of this qualifies as critical, but setting it as such for now, because this seems like too much fragility for something as fundamental as the site's data model.
Comment #2
BerdirI've closed #2225961: Introduce "controlled" setters on FieldDefinitionInterface to avoid special-casing FieldDefinition and #2143297: setters on FieldDefinitionInterface as a duplicate of that this, they might have information that could be relevant.
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedComment #4
xjmDiscussed with @effulgentsia and @alexpott. This is definitely buggy (@effulgentsia pointed out that we have an invalid code example for
hook_entity_base_field_info_alter()
in entity.api.php even), but the actual functional failures are likely edgecases that have workarounds, so this shouldn't block the release of Drupal 8.0.0.Comment #5
heddnThis is still a major issue for all the reasons in #4. But it needs a beta eval and some architectural insight.
Also tagging as related: #2225961: Introduce "controlled" setters on FieldDefinitionInterface to avoid special-casing FieldDefinition
Comment #6
heddnComment #7
heddnComment #8
chx CreditAttribution: chx commentedhttp://drupal.stackexchange.com/questions/156999/creating-account-type-w...
Comment #9
tim.plunkettWe still have in-code @todos pointing to #2225961: Introduce "controlled" setters on FieldDefinitionInterface to avoid special-casing FieldDefinition, which is marked as a dupe of this. Just FYI.
Comment #10
dawehnerCan't you copy an existing one and recreate one?
Comment #11
chx CreditAttribution: chx commentedFor that we would need something like a toArray that you can pass to a factory to facilitate an easier copying.
Comment #15
tstoecklerDiscussed this today with @xjm, @plach, @amateescu and @Berdir and we agreed to downgrade this. There is no functional bug that has been pointed out here, nothing that is actually known not to work, neither in core or in contrib. So we should definitely fix this in some way, but the objects that you get passed do actually have various methods that you need, it's just not typehinted.
We also explicitly agreed that this would be re-raised to major if someone can point out a case where something is being passed that actually does not have the proper methods that you need.
Oh, I think this no longer needs a beta evaluation, by the way ;-)
Comment #16
tstoecklerComment #18
apadernoComment #19
apadernoComment #21
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThese kind of issues are quite complex to unpick, I think it would almost be worth dealing with them in two separate issues. Trying to tackle base fields first.
Reading over the implementation of
EntityFieldManager::buildBaseFieldDefinitions
, things happen in roughly this order:$entity_class::baseFieldDefinitions
is invoked to get the initial list of fields (this hintsFieldDefinitionInterface
).entity_base_field_info
is invoked (also hintsFieldDefinitionInterface
).entity_base_field_info_alter
is invoked (also hintsFieldDefinitionInterface
).Here is my analysis of what the current state of this issue is:
FieldDefinitionInterface
, it doesn't assumeBaseFieldDefinition
outside of a bug in some documentation.BaseFieldDefinition
because that's what everything uses at the moment.FieldDefinitionInterface
that had no setters, except that alter hook implementations would be left scratching their head.I don't believe any of the alter hooks are any less useful hinting
FieldDefinitionInterface
interface. You are still completely able to replace the implementation of a field using setters in an API way like so:Perhaps the issue should be to fix all documentation to make it clear an implementation of
FieldDefinitionInterface
is the ONLY thing required to define a base field and that the interface is what should be expected during alterations.BaseFieldDefinition::createFromFieldStorageDefinition
exists as a helper to create base fields from storage definitions. Perhaps we could create the same thing forFieldDefinitionInterface
. However, given how there aren't many concrete examples of people actually using this flexibility, it might not be worth it.The "special casing" in
EntityFieldManager
seems like it's mostly providing some defaults to BaseFieldDefinitions. I think that's a good enough reason by itself to add setters to the interface, given I don't think it's known yet that an alternative implementation would require such defaults in the first place.Comment #22
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedSo here is a proof of concept for essentially a "wont fix" approach.
The alternative I see is extra setters on
FieldDefinitionInterface
but that could possibly compromise the flexibility of fields coming from a source this is completely immutable. A field definition could start its life out as an instance ofCantBeChangedFieldDefinition
and something could replace it with an instance ofFancyMutableBaseField
and while they are both implementingFieldDefinitionInterface
(and other code expects this), things could continue to work.This is the part of the patch which I think best addresses the concerns which raised this issue in the first place, that is adding special casing for
BaseFieldDefinition
everywhere.Instead of seeing the special casing as a required part of base fields, it'd essentially become a helper for that one type of implementation.
Comment #26
barraponto CreditAttribution: barraponto as a volunteer commentedRe-rolling #22, fixes a couple of test failures by properly updating $fields array in
language_entity_base_field_info_alter
.Also fixes
BaseFieldDefinition::createFromFieldDefinition
to only use methods available inFieldDefinitionInterface
.Comment #29
apadernoComment #30
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedComment #31
xjmI think this would be a minor-only change. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!
Comment #32
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedI've re-rolled patch for 9.1
Comment #35
apadernoThe patch failed for Drupal 9.2.x because an unused
use
statement in /core/modules/language/language.module, which hasn't be changed from the patch.Comment #36
joachim CreditAttribution: joachim as a volunteer commentedThanks for the analysis! Tagging as novice as it sounds like a simple thing to deal with.
Comment #37
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedRe-roll against 9.2.x. Please review.
Comment #38
apaderno@Suresh We need a patch for Drupal 9.3.x.
Comment #39
karishmaamin CreditAttribution: karishmaamin at Specbee commentedRe-rolled patch for Drupal 9.3.x.
Comment #40
daffie CreditAttribution: daffie commentedMissing the parameter in the docblock.
This new method is not used in core anywhere. At least we should add some testing for it.
Comment #44
Nikhil_110 CreditAttribution: Nikhil_110 at Srijan | A Material+ Company commentedAttached patch against Drupal 10.1.x
Comment #46
SpadXIII CreditAttribution: SpadXIII at SIM commentedrerolled against 9.5.x