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)?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fmizzell’s picture

This makes it hard for modules to expose fields to a website

This 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.

sun’s picture

+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.

yched’s picture

Title: Provide a declarative API for modules to define fields without instances » Provide a declarative API for modules to define fields, not just field types

Well 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 :-)

effulgentsia’s picture

Title: Provide a declarative API for modules to define fields, not just field types » Provide a declarative API for modules to define fields without instances

Is this title better?

yched’s picture

Title: Provide a declarative API for modules to define fields, not just field types » Provide a declarative API for modules to define fields without instances

Doh, 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 :-/

sun’s picture

Hm. 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?

swentel’s picture

@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.

Did I misunderstand the goal of this issue?

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

effulgentsia’s picture

Title: Provide a declarative API for modules to define fields without instances » Provide an API for modules to define fields without instances

#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?

yched’s picture

The 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.

sun’s picture

Title: Provide an API for modules to define fields without instances » Improve API for module-created fields (with and without instances)
Category: feature » task

I 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 with strstr($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?

swentel’s picture

Issue tags: +Field API

Tagging.

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.

swentel’s picture

Status: Active » Needs review
FileSize
10.87 KB
26.67 KB

So 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 ?

default-field.png

Status: Needs review » Needs work

The last submitted patch, 1426804-12.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
3.48 KB
11.03 KB

Fixing notices

Status: Needs review » Needs work

The last submitted patch, 1426804-14.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
814 bytes
11.09 KB

Should be green.

swentel’s picture

Removing focus

swentel’s picture

Issue tags: -Field API

Actually removing it ..

andypost’s picture

Issue tags: +Entity Field API

Field addition is going to be a separate task #1963340: Change field UI so that adding a field is a separate task

Berdir’s picture

16: 1426804-16.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 16: 1426804-16.patch, failed testing.

alexpott’s picture

Category: Task » Bug report
Issue summary: View changes
Priority: Normal » Critical
Status: Needs work » Needs review
Issue tags: +Configurables, +D8 upgrade path
FileSize
0 bytes

Reviving 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.

alexpott’s picture

FileSize
20.77 KB

Amateur hour - 0 byte patch in #22.

alexpott’s picture

Title: Improve API for module-created fields (with and without instances) » Allow field storages to be persisted when they have no fields.

#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.

yched’s picture

Wondering 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" ?

Status: Needs review » Needs work

The last submitted patch, 23: 1426804.23.patch, failed testing.

swentel’s picture

Locked 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.

yched’s picture

Right, 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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.34 KB
24.68 KB

Yep 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.

Status: Needs review » Needs work

The last submitted patch, 29: 1426804.28.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2 KB
26.68 KB

Fixed the remaining test fails.

Status: Needs review » Needs work

The last submitted patch, 31: 1426804.31.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
1.44 KB
27.84 KB

And green...

swentel’s picture

Mighty cool! Tested the patch and it works beautifully.

Don't have any big remarks for now, just some nitpicks.

  1. +++ b/core/modules/config/src/Tests/ConfigImportRecreateTest.php
    @@ -85,12 +86,12 @@ public function testRecreateEntity() {
    +    // A node type a field, an entity view display and an entity form display
    

    Misses comma after type

  2. +++ b/core/modules/hal/src/Tests/NormalizerTestBase.php
    @@ -64,6 +64,23 @@ protected function setUp() {
    +    // If the class has node in the list of module install the entity schema
    

    could use a comma after 'module'

  3. +++ b/core/modules/node/src/Tests/NodeBodyFieldStorageTest.php
    @@ -0,0 +1,60 @@
    +    // For module unsintall.
    

    typo

  4. +++ b/core/modules/system/src/Tests/Entity/EntityUnitTestBase.php
    @@ -47,6 +47,24 @@ protected function setUp() {
    +    // If the class has node in the list of module install the entity schema
    

    see above

alexpott’s picture

FileSize
3.29 KB
27.92 KB

@swentel thanks for the review. Fixed everything from #34.

yched’s picture

  1. +++ b/core/modules/field/src/Entity/FieldConfig.php
    @@ -219,7 +219,7 @@ public static function postDelete(EntityStorageInterface $storage, array $fields
    -      if (!$field->deleted && empty($field->noFieldDelete) && !$field->isUninstalling() && count($storage_definition->getBundles()) == 0) {
    +      if (!$field->deleted && empty($field->noFieldDelete) && !$field->isUninstalling() && $storage_definition->isDeleteable()) {
    

    That code block has comment "Delete field storages that have no more fields", that would need to be adjusted ?

  2. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
    @@ -134,6 +134,14 @@ class FieldStorageConfig extends ConfigEntityBase implements FieldStorageConfigI
    +   * Flag indicating whether the field storage should be removed when there are
    +   * no fields.
    +   *
    +   * @var bool
    +   */
    +  protected $persist_with_no_fields = FALSE;
    

    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 ?

  3. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
    @@ -733,4 +741,13 @@ public static function loadByName($entity_type_id, $field_name) {
    +  public function isDeleteable() {
    +    // The field storage is not deleted, is configured to be removed when there
    +    // are no fields and the field storage has no bundles.
    +    return !$this->deleted && !$this->persist_with_no_fields && count($this->getBundles()) == 0;
    

    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) ?

  4. +++ b/core/modules/field_ui/src/FieldOverview.php
    @@ -492,38 +492,23 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
       protected function getExistingFieldOptions() {
    

    Not strictly related, but this method should be renamed getExistingFieldStorageOptions() now. We could do that here ? (well, or not)

  5. +++ b/core/modules/field_ui/src/FieldOverview.php
    @@ -492,38 +492,23 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +    foreach ($this->entityManager->getStorage('field_storage_config')->loadByProperties(array('entity_type' => $this->entity_type)) as $field_storage) {
    

    Rather than the (slower) loadByProperties, we could use EntityManager->getFieldStorageDefinitions($entity_type) and only iterate on configurable storages ?

  6. +++ b/core/modules/field_ui/src/FieldOverview.php
    @@ -492,38 +492,23 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +      // Do not show:
    +      // - locked field_storages,
    +      // - field_storages that should not be added via user interface.
    +      // - where the field is already on the bundle.
    

    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" ?

  7. +++ b/core/modules/field_ui/src/FieldOverview.php
    @@ -492,38 +492,23 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +          'label' => $field_storage->getLabel(),
    

    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 ?

  8. +++ b/core/modules/field_ui/src/Tests/EntityFormDisplayTest.php
    @@ -9,6 +9,7 @@
    +use Drupal\system\Tests\Entity\EntityUnitTestBase;
    

    Not needed

  9. +++ b/core/modules/field_ui/src/Tests/ManageFieldsTest.php
    @@ -12,6 +12,7 @@
    +use Drupal\node\Entity\NodeType;
    

    Not needed

  10. +++ b/core/modules/field_ui/src/Tests/ManageFieldsTest.php
    @@ -72,6 +73,7 @@ function testCRUDFields() {
    +    $this->addingPersistentFieldStorage();
    

    nipick : existing sub-test helpers are doXx(), not doingXx()

  11. +++ b/core/modules/field_ui/src/Tests/ManageFieldsTest.php
    @@ -226,6 +228,30 @@ protected function deleteField() {
    +    // Add a new field based on an existing field.
    

    Copy/pasted from the existing addExistingField(), but could be adjusted here ?
    "Add a new field for the orphaned storage." ?

  12. +++ b/core/modules/hal/src/Tests/NormalizerTestBase.php
    @@ -64,6 +64,23 @@ protected function setUp() {
    +    // If the test installs the Node module we need to install the node entity
    +    // schema. This is because the Node module provides a body field storage
    +    // that would cause tables to be created before the entity tables.
    

    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 :-)

yched’s picture

More 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*" ?

swentel’s picture

So yeah, body should become locked and persistant then ?

Random though: we could do that with title! more formatters and such! ;-)

alexpott’s picture

FileSize
7.76 KB
29.26 KB

#36 @yched thanks for the great review (as always) - and thanks for the for the suggestions - I've used verbatim.

  1. Fixed
  2. Fixed and added a lengthy description
  3. I added the check because it made sense - in case any other code uses this method - not required for the patch to work.
  4. I think it is related sine this method now returns field storages. I stopped short of renaming everything in 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.
  5. Fixed
  6. Fixed
  7. Not sure if this is fixable - we no longer have a field to get the label from here
  8. Fixed
  9. Fixed
  10. Fixed
  11. Fixed
  12. This complexity of installing the node schema is a consequence of the node module providing a default and is only needed if a test base base class installs field configuration and the concrete class is installing the node module. If this is the case we have to create the node entity schema before calling $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

  1. We should do the other conversions in followups - I'll add issues soon.
  2. We can't make them locked because that means in addition to not being able to change anything you can not:
       * - new fields cannot be created
       * - existing fields cannot be deleted.
    

    Both of which we need to do.

re #38 see answer to 37.2 - interesting idea with title.

yched’s picture

Sure, 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 ?

effulgentsia’s picture

Should it also have an effect of preventing the edition of the storage ?

I don't think so. For example, why not let the site builder change the cardinality of node.body?

alexpott’s picture

I agree @effulgentsia I think letting people edit it is ok. As they can't change the field type - everything should be safe.

yched’s picture

Ok, fine by me.

Still need to process #39 - @alexpott, could you fix the comment formatting ? ;-)

alexpott’s picture

Comment formatting in #39 has been fixed - sorry.

yched’s picture

Thanks @alexpott !

New round of review :

  1. +++ b/core/modules/field/src/Entity/FieldConfig.php
    @@ -215,11 +215,12 @@ public static function postDelete(EntityStorageInterface $storage, array $fields
    +    // Delete field storages that have no more fields if the field storage is
    +    // deletable.
    ...
    +      if (!$field->deleted && empty($field->noFieldDelete) && !$field->isUninstalling() && $storage_definition->isDeletable()) {
    

    "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.

  2. +++ b/core/modules/field_ui/src/FieldOverview.php
    @@ -485,45 +486,32 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +      // Do not show:
    +      // - non-configurable field storages.
    +      // - locked field_storages.
    +      // - field_storages that should not be added via user interface.
    +      // - field_storages that already have a field in the bundle.
    

    PÜB, chapter 2 : if the items are uncapitalized, only the last one should have a full stop and the others should have commas ?

  3. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
    @@ -134,6 +134,20 @@ class FieldStorageConfig extends ConfigEntityBase implements FieldStorageConfigI
    +   * By default field storages are removed when there are no remaining fields
    

    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.

  4. getExistingFieldStorageOptions():

    "7. we no longer have a field to get the label from 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 :

    if (the 4 conditions that make the $storage eligible) {
      $options[$field_storage->getName()] = array(
        'type' => ...
        'type_label' => ...
        'field_name' => ...
      );
      if ($arbitrary_bundle = current($field_map[$this->entity_type][$storage->getName()]['bundles'])) {
        $arbitrary_field = EM::getFieldStorageDefinitions($this->entity_type, $arbitrary_bundle)[$storage->getName()];
        $options[$field_storage->getName()]['label'] = $arbitrary_field->getlabel();
      }
    }
    

    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().

  5. The code sample in the previous point made me think of the impact on the "field map" (EM::getFieldmap()).

    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()])).

  6. $this->installEntitySchema('node') :
    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 :-/
alexpott’s picture

FileSize
4.22 KB
29.64 KB

@yched thanks for the review.

  1. Fixed
  2. Fixed
  3. Fixed
  4. Fixed - let's just remove the label
  5. N/a now we are not using the label
  6. I'm not that worried about this - contrib doesn't have to use EntityUnitTestBase - and maybe the comment and block_content will force into doing something that will be this simpler.

Status: Needs review » Needs work

The last submitted patch, 46: 1426804.46.patch, failed testing.

alexpott’s picture

Random fail in Drupal\taxonomy\Tests\TermTest

Test name	Pass	Fail	Exception
CollapsedDrupal\taxonomy\Tests\TermTest	346	1	0
Message	Group	Filename	Line	Function	Status
Autocomplete returns term mUo6WkCB, j7a11rCi after typing the first 3 letters.	Other	TermTest.php	275	Drupal\taxonomy\Tests\TermTest->testNodeTermCreationAndDeletion()	

Status: Needs work » Needs review

alexpott queued 46: 1426804.46.patch for re-testing.

yched’s picture

+++ b/core/modules/field_ui/src/FieldOverview.php
@@ -509,8 +508,7 @@ protected function getExistingFieldStorageOptions() {
         $options[$field_storage->getName()] = array(

Can be $options[$field_name] =... now.

+++ b/core/modules/hal/src/Tests/NormalizerTestBase.php
@@ -64,6 +64,23 @@ protected function setUp() {
+    // If the test installs the Node module we need to install the node entity
+    // schema. This is because the Node module provides a body field storage
+    // that would cause tables to be created before the entity tables.

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 ?

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

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) ?

yched’s picture

Side note, EntityUnitTestBase, like a lot of other tests, still tries to enable entity.module.
Opened #2372477: Lots of tests still enable entity.module

alexpott’s picture

FileSize
2.87 KB
29.83 KB

Addressed #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.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @alexpott. Current patch works for me.
What about the proposal at the end of #50 though ? Would that make sense ?

effulgentsia’s picture

Issue tags: +Needs followup

I 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.

alexpott’s picture

Yep we could do the suggestion at the end of #50 - but we're discussing test code - here let's do that in another issue.

yched’s picture

Yep we could do the suggestion at the end of #50 - but we're discussing test code - here let's do that in another issue.

OK, started it, and it doesn't feel like a clear improvement.

Confirming RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 52: 1426804.52.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
863 bytes
30.67 KB

The 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.

catch’s picture

Minor comments, leaving RTBC since they may all have answers..

  1. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
    @@ -134,6 +134,21 @@ class FieldStorageConfig extends ConfigEntityBase implements FieldStorageConfigI
    +  protected $persist_with_no_fields = FALSE;
    

    Why isn't this just 'persist'?

  2. +++ b/core/modules/field_ui/src/FieldOverview.php
    @@ -485,45 +485,31 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +      if ($field_storage instanceof FieldStorageConfigInterface
    +        && !$field_storage->isLocked()
    +        && empty($field_types[$field_type]['no_ui'])
    +        && !in_array($this->bundle, $field_storage->getBundles(), TRUE)) {
    +        $options[$field_name] = array(
    

    Don't like this long conditional much, but can't really see a nice way to split it up either...

  3. +++ b/core/modules/system/src/Tests/Entity/EntityUnitTestBase.php
    @@ -47,6 +47,26 @@ protected function setUp() {
    +    // If the concrete test sub-class installs node.module, ensure that the node
    

    This appears twice, could we move it to a helper somewhere?

alexpott’s picture

Thanks 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.

yched’s picture

the issue to make all tables lazy create could make this obsolete

Ooh - what's that issue ?

catch’s picture

#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.

  • catch committed cd9b0f4 on 8.0.x
    Issue #1426804 by alexpott, swentel: Fixed Allow field storages to be...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Alright, committed/pushed to 8.0.x. Thanks!

yched’s picture

@catch:

Which issue is the follow-up? If that follow-up can be re-purposed to remove this code that sounds ideal

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

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) ?

- 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 :-)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.