Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Field API deletes fields when the last instance is deleted. Field UI populates the "add existing field" dropdown only with fields with at least one instance. This makes it hard for modules to expose fields to a website, and build business-logic functionality tied to that field, without also locking at least one bundle into using (instantiating) that field. I just posted a D7 contrib module, ModuleField, for solving this. Is there any interest in this being added to Drupal core (directly to field.module and field_ui.module)?
Comment | File | Size | Author |
---|---|---|---|
#58 | 1426804.58.patch | 30.67 KB | alexpott |
#58 | 52-58-interdiff.txt | 863 bytes | alexpott |
#52 | 1426804.52.patch | 29.83 KB | alexpott |
#52 | 46-52-interdiff.txt | 2.87 KB | alexpott |
#46 | 1426804.46.patch | 29.64 KB | alexpott |
Comments
Comment #1
fmizzell CreditAttribution: fmizzell commentedThis is true, but I am wondering if there is a purpose for exposing fields to a website beyond the already mentioned case of adding business-logic that is tied to a field.
If that is the case, #1803064: Horizontal extensibility of Fields: introduce the concept of behavior plugins would solve that problem, and it would make the logic reusable on any field of the type or types for which the behavior was designed, not just a specific field exposed by a module.
There might be other reason why this might be useful, but for reusable business logic attached to fields, the previously mentioned issue is a more flexible solution.
Comment #2
sun+1 — We could really use such functionality for #1751210: Convert URL alias form element into a field and field widget
The use-case is that Path module wants to provide a default "path" field, which should just exist by default, and which may be added to individual entity bundles. The field should not even be pruned/deleted when the last instance was removed.
Comment #3
yched CreditAttribution: yched commentedWell the thing is that fields and instances are very much about an imperative API. They have a CRUD cycle that we need to react to.
The fact that "pseudo fields", OTOH, currently come and go silently in hooks is something that we need to hack our way around in some places.
Besides,
- $field and $instance are moving to CMI config entities. I don't see us introducing an additional pattern for configuration items to enter the system by being exposed in 'hook_default_X' hooks ? (that pattern has just been removed in views, btw)
- #1852966: Rework entity display settings around EntityDisplay config entity splits the 'display' part of $instance out to a separate EntityDisplay config object - with its own declarative API. $instance['widget'] will follow the same path. So even with a declarative API on $field and $instance, you'll still need a separate imperative sequence to configure forms and displays. Unless we introduce hooks for exposing any kind of entity ?
The issue title makes it sound like "field types" and "fields" are almost the same thing - "what we do with one, why couldn't we do with the other ?". That's, er, totally not the case :-)
Comment #4
effulgentsia CreditAttribution: effulgentsia commentedIs this title better?
Comment #5
yched CreditAttribution: yched commentedDoh, I mistook this issue for "add fields and instances in hooks", and that was mostly what #3 replied to.
Sorry about that.
I'd need to look into http://drupal.org/project/modulefield more closely.
But, last time I had to deal with that, fields without instances were kind of a pain to deal with :-/
Comment #6
sunHm. I'm not sure about the "declarative" part. At least I don't see why it would have to be declarative. Also not sure about the "without instances" part ;) Perhaps my interest is different to this issue's goal after all? Let me try to explain:
1) As a module developer, I want to be able to install a field with default settings when my module is installed, so the field can be attached to bundles, if desired.
2) As a module developer, I also want to be able to automatically install field instances of the field that I installed. Those instances may be customized and can be removed by the site admin later on, potentially even all, but (TBD) the field should still persist.
3) As a module developer of a very specific field type, I want to be able to define and supply default settings for every (new) instance of a field, or have some other way to (help to) ensure a consistent appearance of all field instances across entities/bundles. (Concrete example: the URL alias field.)
4) As a module developer, I'd also like to automatically attach a field to every new bundle that is created for a particular entity type. (Again, concrete example: Every new node/content type should automatically get the URL alias field by default - perhaps even all bundles ;))
So I'm definitely not interested in declaring/defining fields + instances via runtime code. :) I still see the regular field/instance CRUD happening, I just want/need better methods for performing these field type 1:n bundles operations on the API level, module installation, and installation profile level, as well as some special handling for these module-created fields/instances with regard to their lifecycle.
Did I misunderstand the goal of this issue?
Comment #7
swentel CreditAttribution: swentel commented@sun 1 and 2 are going to work after the field api cmi conversion (tests in that patchare proving that in a way). 3 and 4 will need some coding, but hook_node_type_create/update should work find after node types are converted to config.
I think the real goal here is to define 1 field without an instance that you can select automatically on field UI ? Unless I'm missing something too here ? Or does it even go further like: one field automatically creates the instance as well ? Again, I think that would also be solved again with field api on cmi - in a way. And otherwise, adding logic to field ui shouldn't be *that* hard I think.
- edit - 3 needs some custom coding as well
Comment #8
effulgentsia CreditAttribution: effulgentsia commented#6 captures the goals I originally had for this issue. Please retitle as you see fit. Good news on #7; does it make sense to postpone this on #1735118: Convert Field API to CMI?
Comment #9
yched CreditAttribution: yched commentedThe problem with fields without instances is that Field UI really lets you work with instances, and hides the standalone notion of fields completely. Users only have to manage their instances, they don't have to manage their fields, this is taken care of.
This matches most actual use cases and hides some complexity.
If we allow "fields without instances", then it means the UI needs to account for managing fields as well - like:
- if you delete the last instance, do you want to keep the field around ? (95% cases: no)
- how do you create/edit/delete a field that's not attached to an actual entity bundle ?
- can you delete a field while it still has instances ?
That's a lot more cases to deal with. And Field UI only attaches to "per bundle" pages - we don't have a convenient place for "cross entity types / cross bundles" notions in our IA.
Comment #10
sunI think that there is one aspect that could potentially help to achieve a different behavior for such fields:
All custom/user-created fields are prefixed with
'field_'
, whereas module-created fields are not (e.g.,'body'
,'path'
, etc).Ultimately, I think what we're aiming for is a slight deviation of non-custom, module-created fields with regard to their lifecycle/behavior. AFAICS, the only indicator for that is the
'locked'
property currently, but that has a special meaning — we likely want to introduce a new property, unless we're OK withstrstr($field_name, 'field_') !== FALSE
conditions.I actually don't think that the needed changes would be very large...
- First and foremost, we'd have to give
field_create_field()
a meaning of its own. I was very surprised to find out that creating a field (without instance) has no effect at all except of creating a field config entry and that just creating a field is completely pointless currently. ;) Off-hand, I can't see how #1735118: Convert Field API to CMI would change or improve that situation, since it seems to be FieldOverview that explicitly filters out all fields that don't have an instance. Removing that filter would be the first task.- Second, to achieve default instance settings (3), we might want to introduce exactly that for field definitions? I.e., allow fields to suggest a default widget, formatter, settings, and label and stuff? (not in metadata, just introduce a new, optional method, since this info is only needed when the field is instantiated already and when possibly creating an instance, and could very well involve dynamic/conditional logic)
Lastly, for point #6.4, I'm actually not sure whether we need to do anything — in case the Bundle CRUD API fires a hook upon creating a new bundle, then modules like Path module might be able to do what they want to do already?
Comment #11
swentel CreditAttribution: swentel commentedTagging.
Also, CMI conversion is in, so we can start thinking about this again :)
@sun You're right indeed. Only shipping a field will not expose this in the UI and, as you mentioned, I think that could be a fairly small patch to begin with.
Comment #12
swentel CreditAttribution: swentel commentedSo this works: you can now ship a field in config without instance and then select it in Field UI. #1963340: Change field UI so that adding a field is a separate task might become very handy now ;)
You can test it by enabling field_test_config
TBD: what if the last instance is deleted ? Keep with some special property ?
Comment #14
swentel CreditAttribution: swentel commentedFixing notices
Comment #16
swentel CreditAttribution: swentel commentedShould be green.
Comment #17
swentel CreditAttribution: swentel commentedRemoving focus
Comment #18
swentel CreditAttribution: swentel commentedActually removing it ..
Comment #19
andypostField addition is going to be a separate task #1963340: Change field UI so that adding a field is a separate task
Comment #20
Berdir16: 1426804-16.patch queued for re-testing.
Comment #22
alexpottReviving this issue and making it critical because it blocks being able to deploy node types that use the default body field. The auto creation in ndoe_add_body_field() can not be deployed and because we have book, forum, standard and the UI all relying on a common body field we have a problem. This issue is now a precursor to #2321385: Creation of node body field in postSave() incompatible with default config and overrides - where we will move most of the default configuration into default config files. This issue focused on allowing node to create the field body storage and how we maintain a field storage without any instances.
The patch takes a completely different direction from previous patches since it relies on the configuration system to deliver the default field storage configuration - so no interdiffs.
I would not be surprised if there are test fails due to fun with the abstract EntityUnitTestBase and the fact in installs field configuration. If the node module is being enabled in the test then the entity schema has to be installed before the node module's field config - this is an oddity related to how KernelTestBase ignores module dependencies and does not install all schema.
Comment #23
alexpottAmateur hour - 0 byte patch in #22.
Comment #24
alexpott#2321385-43: Creation of node body field in postSave() incompatible with default config and overrides outlines why this is a critical bug.
Fixing this bug has the side effect of improving the API for module providing field storages.
Comment #25
yched CreditAttribution: yched commentedWondering what this means in combination with the existing 'locked' property ?
I'm not sure we currently have any semantics attached to 'locked' on FieldStorageConfig. Could it be what 'locked' means for storages - "cannot be deleted even when no field uses it" ?
Comment #27
swentel CreditAttribution: swentel commentedLocked only seems to be for Field UI - and even then you can still change the cardinality, which is probably a bug - see #2274433: Do not allow to alter Locked field via UI. But the API doesn't care about the locked property at all.
Comment #28
yched CreditAttribution: yched commentedRight, sorry, mixed things up with "status", which is a generic property provided by the config system for all ConfigEntities, but which exact semantics is left to each entity type.
I thought "locked" was the same (IIRC this was discussed at some point), but it's not.
So yeah, we do have an explicit "locked" property on FieldStorage right now, and it's fully not clear what it does exactly.
Comment #29
alexpottYep unfortunately the locked key already has a lot of meaning - like you can not add anymore for fields for that storage and so is completely incompatible with what we want to do here.
Patch fixes some tests and the incorrect assumption that
noFieldDelete
was a field storage property - it was not - it was a field runtime property. Completely undocumented but that is a separate issue.Comment #31
alexpottFixed the remaining test fails.
Comment #33
alexpottAnd green...
Comment #34
swentel CreditAttribution: swentel commentedMighty cool! Tested the patch and it works beautifully.
Don't have any big remarks for now, just some nitpicks.
Misses comma after type
could use a comma after 'module'
typo
see above
Comment #35
alexpott@swentel thanks for the review. Fixed everything from #34.
Comment #36
yched CreditAttribution: yched commentedThat code block has comment "Delete field storages that have no more fields", that would need to be adjusted ?
Yeah, hard to phrase under 80 chars...
Suggestion: "Flag indicating whether the field storage should be deleted when orphaned." ?
Also, could use a word of explanation on which use cases this is useful for / why would I want to set it to TRUE ?
Just curious : the previous code in FieldConfig::postDelete() (that now calls this method) did not check for $storage->deleted.
Was there a specific reason this was added, or was it just because it made sense (which it does) ?
Not strictly related, but this method should be renamed getExistingFieldStorageOptions() now. We could do that here ? (well, or not)
Rather than the (slower) loadByProperties, we could use EntityManager->getFieldStorageDefinitions($entity_type) and only iterate on configurable storages ?
The phrasing for the last item is weird, it sounds like it's ((1 || 2) && 3), while it's actually (1 || 2 || 3)
--> "- field_storages that already have a field in the bundle" ?
Behavior change : This will use the label of the storage, which is its id ("node.body"). Existing code uses the label of the field, which is a human label - meaning, we currently pick the label of one of the storage's fields for displaying in the dropdown. Not sure we want to change this here ?
Not needed
Not needed
nipick : existing sub-test helpers are doXx(), not doingXx()
Copy/pasted from the existing addExistingField(), but could be adjusted here ?
"Add a new field for the orphaned storage." ?
What this means is installConfig('node') has to run before installEntitySchema('node') ?
Yet that base test class doesn't do the installEntitySchema('node'), it's the concrete test subclasses that do, so couldn't it be their job to call installConfig('node') as well, instead of adding that somewhat complex code in two places (possibly more in the future) ?
On that topic, I don't fully get all the $this->installEntitySchema('node') removals in other test classes, so yeah, I must be missing something here :-)
Comment #37
yched CreditAttribution: yched commentedMore general remarks (d.o went all 500 yesterady just after I posted #36) :
- We'll need to do the similar for:
comment body (CommentManager::addBodyField())
custom_block body (block_content_add_body_field())
Here or followup ?
- The connection with a notion of "locked" is that the current code here still lets you edit the 'body' field storage in the UI. If the feature we shoot for here is "node.module provides the definition for the 'body' storage, and it sticks around no matter which fields get added / removed in bundles", then shouldn't it be also "and it stays *as node.module defined it*" ?
Comment #38
swentel CreditAttribution: swentel commentedSo yeah, body should become locked and persistant then ?
Random though: we could do that with title! more formatters and such! ;-)
Comment #39
alexpott#36 @yched thanks for the great review (as always) - and thanks for the for the suggestions - I've used verbatim.
FieldOverview::buildForm()
because I think the terminology of "existing field" is okay here and replacing "field" with "field storage" will end up in the user's face and make things more difficult rather than simpler.$this->installConfig(array('field'));
. Afaics there is no other way around this other than moving node into the list of installed modules in EntityUnitTestBase and always installing the node entity schema - happy to do that if you think this is preferable.re #37
Both of which we need to do.
re #38 see answer to 37.2 - interesting idea with title.
Comment #40
yched CreditAttribution: yched commentedSure, our current behavior for "field.storage:locked" cannot be applied here.
What muddies the waters is that the "field.storage:locked" feature :
- is currently kind of broken (prevents edition / creation / deletion of *fields*, but lets you edit the *storage*... that's #2274433: Do not allow to alter Locked field via UI)
- seems kind of stale with the concept of code-defined fields in D8 anyway ?
Let's leave "locked" as it currently exists out of the equation here. My question is : should the UI let you edit the field storages we're talking about in this issue here (node body, comment body...) ?
What this issue adds is a kind of configurable fields that behaves slightly differently from the usual, for the use case of "I want my entity type to provide one fixed notion of a "body" field, and leave it to admins to use it or not in the actual bundles".
This is done by adding a new flag on the field.storage entities. This flag currently only has the effect of not deleting the storage if it becomes orphaned. Should it also have an effect of preventing the edition of the storage ?
Comment #41
effulgentsia CreditAttribution: effulgentsia commentedI don't think so. For example, why not let the site builder change the cardinality of node.body?
Comment #42
alexpottI agree @effulgentsia I think letting people edit it is ok. As they can't change the field type - everything should be safe.
Comment #43
yched CreditAttribution: yched commentedOk, fine by me.
Still need to process #39 - @alexpott, could you fix the comment formatting ? ;-)
Comment #44
alexpottComment formatting in #39 has been fixed - sorry.
Comment #45
yched CreditAttribution: yched commentedThanks @alexpott !
New round of review :
"Painful überpicky asshole" time : looking at the code, it's "Delete field storages that are deletable()", deletable() enclosing "has no more fields, and is not configured to persist".
So maybe "Delete the associated field storages if they are not used anymore and are not persistent" ? This expands the logic of isDeletable() here, but this is where the housekeeping is made, so making the logic explicit might be best.
PÜB, chapter 2 : if the items are uncapitalized, only the last one should have a full stop and the others should have commas ?
PÜB, the final battle : we should probably be specific that this is only about configurable field storages, we're not talking about FieldStorageDefinitionInterface in general here.
Ah, true. Well, then displaying a "label" that is just the storage ID is useless. Going at admin/structure/types/manage/page/fields, the current code displays options like "[field type]: [field name] (node.[field name])". The parenthesis doesn't really add valuable info, I know I'm working on node fields ?
We could keep displaying a human label if we can have one, with :
and adjust the consuming code for the case where no 'label' is present ?
But well, then some options show a human label between parenthesis, and some don't. No biggie IMO, but well, alternatively, we just drop that parenthesis.
Also nitpick: now that the code uses EM::getFieldStorageDefinitions(), it could use the keys of the returned array as $field_name rather than repeatedly calling $storage->getName().
With current patch, if I delete all body fields, EM::getFieldMap()['node'] contains no entry at all for 'body'.
That's probably OK, since it's a *field* map, and the phpdoc for EM::getFieldmap() says "Returns a lightweight map of fields across bundles". If a storage exists but isn't used by any field in any bundle, then it's not in the "map of fields across bundles", fine.
It just means the sample code above will need an isset($field_map[$this->entity_type][$storage->getName()])).
Damn, tricky. No real opinion atm, but I'm worried that we'll need to do more of that ugly stuff for 'comment' and 'block_content' entity types when we deal with their body fields - and possibly other fields / entity types in the future ? Also, contrib entity types will have to figure similar (highly non-trivial) workarounds for their own tests if they hit the case :-/
Comment #46
alexpott@yched thanks for the review.
Comment #48
alexpottRandom fail in
Drupal\taxonomy\Tests\TermTest
Comment #50
yched CreditAttribution: yched commentedCan be $options[$field_name] =... now.
Really sorry to be a pain about this, but even re-reading the code comment here and the explanations in the issue, I'm still having troubles understanding the full picture here, so this feels like a hairpuller we leave for the next guy that has to touch this area.
- The comment should probably make it clearer that all this is about running $this->installEntitySchema('node') before installConfig(array('field')).
Proposal: "If the concrete test sub-class installs node.module, ensure that the node entity schema is created before the field configurations are installed, because the node entity tables need to be created before the body field storage tables."
- Not clear what does break if the body field storage tables are created before the entity schema ? Is that something that could be added to the comment ?
Agreed that it wouldn't be too great. Alternatively, why don't we leave it to each subclass to run installConfig('field') ? What this does is import [enabled_module]/config/install/field.*.yml, right ? But none of the modules enabled by the abstract EntityUnitTestBase test class seems to have any of those, so installConfig('field') is only relevant if the concrete subclass needs it for the additional modules it installs (e.g node) ?
Comment #51
yched CreditAttribution: yched commentedSide note, EntityUnitTestBase, like a lot of other tests, still tries to enable entity.module.
Opened #2372477: Lots of tests still enable entity.module
Comment #52
alexpottAddressed #50.
What breaks is that we try to create the body field tables twice. This is a dependency problem created by the strangeness of KernelTestBase.
Comment #53
yched CreditAttribution: yched commentedThanks @alexpott. Current patch works for me.
What about the proposal at the end of #50 though ? Would that make sense ?
Comment #54
effulgentsia CreditAttribution: effulgentsia commentedI think it's important that we have a followup issue to add docs to field_create_field() on whether/how it's safe to use without running into the same problems that prompted this issue being raised to critical in the first place. For now, just tagging to not forget.
Comment #55
alexpottYep we could do the suggestion at the end of #50 - but we're discussing test code - here let's do that in another issue.
Comment #56
yched CreditAttribution: yched commentedOK, started it, and it doesn't feel like a clear improvement.
Confirming RTBC.
Comment #58
alexpottThe changes to
Drupal\views\Tests\Entity\ViewEntityDependenciesTest
in #2267453: Views plugins do not store additional dependencies mean we need to install some config and tables to make the test work now that the body storage is provided by the node module as configuration.Setting back to rtbc as the change has little to do with the logic of the patch.
Comment #59
catchMinor comments, leaving RTBC since they may all have answers..
Why isn't this just 'persist'?
Don't like this long conditional much, but can't really see a nice way to split it up either...
This appears twice, could we move it to a helper somewhere?
Comment #60
alexpottThanks for the review @catch
1. To be super clear on the difference with locked
2. Me neither
3. This is test code - Yched has already been looking at refactoring in a follow up. Plus the issue to make all tables lazy create could make this obsolete.
Comment #61
yched CreditAttribution: yched commentedOoh - what's that issue ?
Comment #62
catch#1 fair enough I guess.
#2 OK, shouldn't hold this up.
#3 / @yched that's #2371709: Move the on-demand-table creation into the database API which is RTBC (but I've not done a full review yet). Which issue is the follow-up? If that follow-up can be re-purposed to remove this code that sounds ideal.
Comment #64
catchAlright, committed/pushed to 8.0.x. Thanks!
Comment #65
yched CreditAttribution: yched commented@catch:
So yeah, wasn't too clear in the earlier comments, backtracking this :
- in #50 I suggested a different approach for the issue about installEntitySchema('node') / installConfig('field') order
- in #50 @alexpott said why not, but followup plz
- in #56, I said I gave it a try, and didn't feel it was a clear improvement.
So in short, that code in NormalizerTestBase / EntityUnitTestBase still bugs me, but I have no better proposal :-/
Other than that, yay for commit :-)