Problem/Motivation

It seems like FieldStorageDefinitions are not created for fields provided by hook_farm_entity_bundle_field_info() until a new entity of that type is installed. The field definition is created and can will become visible in the UI, but trying to save data into that field will result in a 500 with an error like:

Drupal\Core\Entity\EntityStorageException: SQLSTATE[42P01]: Undefined table: 7 ERROR: relation "public.log__equipment" does not exist LINE 6: AND pg_attribute.attrelid = 'public."log__equipment"'::regcl... ^:

I tried clearing the cache many times throughout this process.

This is problematic because if a module is installed at some point after farmOS Core was installed, any bundle field definitions it provides will not be created.

It seems that this happens because `hook_entity_field_storage_info` is not invoked at the same time hook_entity_bundle_field_info is invoked. The contrib Entity module calls the $bundle_handler->getFieldStorageDefinitions and $bundle_handler->getFieldDefinitions methods in its respective implementations of these hooks.

I haven't tested all of the edge cases, but it seems that installing a new bundle of the entity AFTER the module is installed triggers hook_entity_storage_info and then allows the module to be installed.

Steps to reproduce

Install farmOS without the farm_equipment module. Then install the farm_equipment module. Create an equipment asset, and then try to create a log that references the equipment asset. It should fail.

Install a new log type, then saving the previous log with an equipment reference will work.

Proposed resolution

Remaining tasks

Write tests that recreate this behavior.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Comments

paul121 created an issue. See original summary.

paul121’s picture

paul121 commented
Issue summary: View changes
paul121’s picture

paul121 commented

I wrote a test that does a "postponed install" of the Equipment module: https://github.com/paul121/farmOS/commits/2.x-bundle_field_storage

I believe that using $moduleInstaller->install() is the correct way to test this "postponed install", but I'm not 100% sure. It is the same service that installs modules from a test's $modules property, so it seems fair enough.

Results in a failing test case either way: https://github.com/paul121/farmOS/runs/1685041886?check_suite_focus=true

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Priority: Normal » Critical
paul121’s picture

paul121 commented

This is happening due to an incorrect provider being set on the field storage definitions provided by hook_farm_entity_bundle_field_info.

The Drupal core ModuleInstaller loops through all field storage definitions and only installs the ones provided by the module that is currently being installed: https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/lib/Drupal/C...

Since these fields are created by the contrib Entity BundlePluginHandler in its implementation of hook_entity_field_storage_info the provider is considered to be entity, not the module that implemented our hook. Unfortunately we can't change this early on - the provider is hard coded by the core EntityFieldMangager::buildFieldStorageDefinitions method: https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/lib/Drupal/C... (Unsurprisingly, there is a note in the comments to change this per #2225961: Introduce "controlled" setters on FieldDefinitionInterface to avoid special-casing FieldDefinition, which is related to other pending core entity bundle field-related issues)

...but we do have a chance to alter the storage definition! That's the simplest solution I see here; alter the field storage definitions provided by hook_farm_entity_bundle_field_info to set the correct provider. This way DB storage will be created right when these modules are installed. This also handles uninstalling a module - right now when farm_equipment is uninstalled, the log__equipment table remains, which is another bug.

In addition to the equipment "postponed" test, I added a test in the core farm_entity module to check that the correct provider is set. Considering the upstream logic in core, I think that should be sufficient.

Failing tests: https://github.com/paul121/farmOS/runs/1705267433
Passing with the fix: https://github.com/paul121/farmOS/runs/1705303564

paul121’s picture

paul121 commented
Status: Active » Needs review

Oh also related, I wanted to mention how the contrib Entity module works around this for entity bundles in general. It's a similar issue, but not exactly the same. I'm assuming that since bundles are normally defined as config, the core ModuleInstaller doesn't check for any bundle related field storage data. So instead of being able to do some altering like we can do on a FieldStorageDefinition, they manually install & and uninstall the bundle field definitions in hook_modules_installed and hook_modules_preuninstall with a custom BundlePluginInstaller service: https://git.drupalcode.org/project/entity/-/blob/8.x-1.x/src/BundlePlugi...

It seems that we could go a similar route implementing hooks on install/uninstall, check if the module provided a bundle field, and act accordingly... but we get all this behavior "for free" by just altering the field definitions to have the correct provider. This is a hack and isn't as explicit as code in separate install/uninstall hooks, but the simplicity seems to be worth it IMO.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Status: Needs review » Needs work
Parent issue: » #3151233: [META] farmOS 2.x Data Model
Related issues: +#2346347: Finalize API for creating, overriding, and altering code-defined bundle fields

Great sleuthing @paul121 - thanks for the tests, fix, and detailed explanations!

Still wrapping my head around how the Entity API module itself handles this - and why it doesn't handle our case...

I think the hook_entity_field_storage_info_alter() approach to fixing this will work fine, but as you said "This is a hack and isn't as explicit as code in separate install/uninstall hooks". I'm not too concerned with the "hackiness" of this particular fix - but I do want to make sure that whatever approach we take is easy to unwind in the future, if/when Drupal core and/or Entity API change the way they handle these bundle fields.

Two other thoughts/questions about your branch:

1. Can we generalize and move the EquipmentFieldPostponedInstallTest code into the farm_entity module? It seems like something we should be testing more generally, since it is not necessarily specific to the Equipment module (just happens to be the only place farmOS core is using hook_farm_entity_bundle_field_info() currently). The farm_entity_test module also implements this hook, so I think that would give us something to test against more generally?

2. The last commit in the branch seems unrelated: "Replace FieldDefinitionInterface with BaseFieldDefinition." And I'm not sure we actually want to do that. The reason I used FieldDefinitionInterface is because FarmFieldFactory actually has to produce two different output object classes: BaseFieldDefinition and BundleFieldDefinition. So by using FieldDefinitionInterface it covers both. I agree this isn't perfect, and the IDE type-hinting issue you mention is a side-effect of that. But it may be a necessary evil until (if) core adopts a unified field definition class for base and bundle fields.

This is the upstream core issue we should be tracking alongside all these decisions: #2346347: Finalize API for creating, overriding, and altering code-defined bundle fields

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

But it may be a necessary evil until (if) core adopts a unified field definition class for base and bundle fields.

It may be worth creating a dedicated (postponed) issue in our queue to track #2346347: Finalize API for creating, overriding, and altering code-defined bundle fields, and document all the considerations it has on our hook_farm_entity_bundle_field_info(). As time goes by we will definitely forget all these things. Organizing these thoughts in advance will be doing our future selves a favor. :-)

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Related issues: +#3194206: Refactor bundle fields when Drupal core supports code-defined bundle fields

It may be worth creating a dedicated (postponed) issue

Done: #3194206: Refactor bundle fields when Drupal core supports code-defined bundle fields

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

Oh also just noticing... in that core code you linked to (https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/lib/Drupal/C...):

          if ($definition instanceof BaseFieldDefinition) {
            $definition->setProvider($module);
          }

That is explicitly checking to see if the $definition is an instance of BaseFieldDefinition. But fields created by our hook_farm_entity_bundle_field_info() are instances of BundleFieldDefinition. So that $definition->setProvider($module); code probably isn't running at all, which means a provider isn't being set. Is that correct? Or am I misreading?

If that's true, does that make it any easier for us to set the provider somewhere else? Like in FarmEntityBundlePluginHandler?

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

If that's true, does that make it any easier for us to set the provider somewhere else? Like in FarmEntityBundlePluginHandler?

I just pushed a commit to test this hypothesis - let's see what happens: https://github.com/mstenta/farmOS/actions/runs/503812076 (edited with new link).

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

My test did not fix the issue - probably stemming from the way core works? I may try to dig in a bit more to understand why...

But I wonder: should we be setting the provider like that anyway? Even if it has no effect now, will it in the future? Is that the "correct" thing to do, despite the "incomplete" nature of core's support for bundle fields?

paul121’s picture

paul121 commented

Since BundleFieldDefinition extends from BaseFieldDefinition, this means that BundleFieldDefinition is an instance of BaseFieldDefinition as well.

instanceof can also be used to determine whether a variable is an instantiated object of a class that inherits from a parent class:

Double checked this to confirm :-) - https://www.php.net/manual/en/language.operators.type.php

I think this should clear up your point #2:

The reason I used FieldDefinitionInterface is because FarmFieldFactory actually has to produce two different output object classes: BaseFieldDefinition and BundleFieldDefinition. So by using FieldDefinitionInterface it covers both.

As well as explain how that core code is functioning:

But fields created by our hook_farm_entity_bundle_field_info() are instances of BundleFieldDefinition. So that $definition->setProvider($module); code probably isn't running at all, which means a provider isn't being set. Is that correct? Or am I misreading?

This code should be running. I confirmed this at one point when I saw that the log.equipment field's provider was entity.

I was hoping we could simply set the correct provider from the beginning, but it led me into core where this is hard coded (mentioned above):

Unfortunately we can't change this early on - the provider is hard coded by the core EntityFieldMangager::buildFieldStorageDefinitions method:

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

Since BundleFieldDefinition extends from BaseFieldDefinition, this means that BundleFieldDefinition is an instance of BaseFieldDefinition as well.

Ah! TIL that PHP's instanceof also matches child classes. Didn't know that!

That explains why my fix didn't work. Core still picked it up and replaced the provider.

I think this should clear up your point #2:

It does! You're right. We can include that commit.

Aside from moving the test (which is a nitpick), I think this is good to go! I may also add some links to the new issue (#3194206: Refactor bundle fields when Drupal core supports code-defined bundle fields) in our code so that we know where to pick up again in the future as things evolve upstream.

paul121’s picture

paul121 commented
Status: Needs work » Needs review

Sounds good @m.stenta! Additional comments in the code would be good.

I moved the postponed installation test to the farm_entity module, rebased, and force pushed to my branch. Tests are passing, should be good to go: https://github.com/paul121/farmOS/commits/2.x-bundle_field_storage

  • m.stenta committed b67aa98 on 2.x 
    Issue #3192245 by paul121: Storage is not created for fields created...
m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Status: Needs review » Fixed

Thanks @paul121 - merged!

Status: Fixed » Closed (fixed)

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