Provide a plan_record entity type for plan record relationships with metadata

[mstenta]

This is a proposed solution to Issue #3187877: [META] Plan entity relationships with metadata. Read that thread for the full context and problem statement.

This adds a new plan_record entity type, as an optional sub-module of the plan module. The plan_record entity type supports sub-types (bundles), and bundle plugin support is provided by the farm_entity module, so that modules can declare plan_record bundles and their bundle field definitions in PHP files similar to asset, log, and plan types.

The implementation is very simple right now. There is no UI or access logic included. The expectation is that these entities will be used primarily by custom code in specific planning modules, which will be responsible for creating and maintaining them for their purposes.

We may want to add some core access logic in the future to support API-based use-cases, but that is not a requirement right now so I kept this barebones so we can consider it.

This new plan_record entity type is currently being used in the 3.x-plan-record branch of the farm_crop_plan module, which is a v3 port of the crop planning module that we had start in farmOS v1: https://github.com/mstenta/farm_crop_plan

Here is an example of a bundle of the plan_record entity type that the farm_crop_plan module provides: https://github.com/mstenta/farm_crop_plan/blob/10862066d473c50d3a09ba4c583b853228270009/src/Plugin/PlanRecord/PlanRecordType/CropPlanting.php

The crop planning module adds a crop_planting bundle of the plan_record entity type, which is used to link a plant asset to a crop plan, along with metadata like seeding_date, days_to_transplant, days_to_harvest, and harvest_window. Note that most of these represent the "planned" seeding/transplanting/harvest details, but the module will use seeding/transplanting/harvest logs to represent the "actual" details. This allows capturing and highlighting the differences between "planned" vs "actual", which is useful information to have and highlight in a crop planning UI.

One aspect of this new plan_record entity type's data model that I want to highlight is: the plan_record entity reference the plan entity. Not the other way around. This is the opposite relational direction of the asset and log reference fields that are added to plan entity bundles by default (note that the crop plan overrides the bundeFieldDefinitions() method so it doesn't use those two default fields).

I considered approaching this the other way as well. It would be possible to require plan entities to reference plan_record entities instead, via one or more entity reference fields on the plan entity itself. This would be more similar to the default asset and log reference fields on plan entities, or similar to the way that a log entity references quantity entities.

The biggest disadvantage of that approach is that it means you need to manage both the plan_record entities and the references to them. For example, if a plant asset is removed from a plan, you would need to delete the plan_record entity, then delete the reference to it. There's a risk of having "orphaned" plan_record entities in the database if they aren't carefully managed (we are currently dealing with that already with quantities: #775). This could lead to issues deleting records that are referenced by the orphaned plan_record, due to entity reference integrity constraints. On the other hand, if the plan_record has a required plan reference, it isn't possible for it to be orphaned.

The other benefit of plan_record entities referencing plan entities is that we can more easily leverage Migrate API to build CSV importers/updaters for plans. I'm excited to explore this in the farm_crop_plan module, to make it easier to build and update crop plans in a spreadsheet, and just use farmOS for the visualization and logging. I'm starting to experiment with that here: https://github.com/mstenta/farm_crop_plan/blob/a34a4574a01ddfc80a98d865ecb202fa9b024a9b/migrations/crop_plan_record.yml

Notice that the destination.plugin is entity:plan_record, which means it is importing/updating plan_record entities directly, and keying off of the plan ID and plant asset ID to recognize if it's an new/existing record. This would be much more difficult to achieve if the plan references the plan_record entities, because it would also have to be concerned with updating the reference field on the plan.

Curious to hear what others think of this approach, and if there are any concerns or considerations I might be overlooking!

I will leave this in draft status for now... if we adopt it we'll also want to add data model docs for the plan_record entity type, and maybe some more elaborate usage documentation (although that could also wait until we have more real world examples too).

[wotnak]

Really like this approach.

Was experimenting some time ago with a similar idea for relationships between logs and assets.

It allows adding new and more complex relationship types without adding more and more fields to main entities. When adding a new relationship type, the database structure changes are more contained and don't directly affect entity types the relationship record references. So with this approach, contrib modules that are adding relationship types no longer will need to modify base entity types by adding new fields to them.

In my opinion, it's worth considering that this approach could be useful for relationships between other entity types. To that end, maybe the relationship record entity could be made more generic so in the future it could be used in other places, not just for plans.

[mstenta]

In my opinion, it's worth considering that this approach could be useful for relationships between other entity types.

That's interesting @wotnak! If we took that approach, I wonder what we would call the entity type. 🤔

Maybe just relationship? Or relationship_metadata? Or just metadata? I'm also reminded of this old project: https://www.drupal.org/project/relation/

It would need to be an entity type with no base fields, in order to be able to support referencing any entity type. So it would be up to each bundle to add entity reference bundle fields. The crop_planting bundle, for example, would add a plan reference field and a plant asset reference field (as opposed to the current implementation, which adds a plan reference field to all bundles, and crop_planting just adds plant).

Worth considering! It could be a very flexible option. Although maybe it runs the risk of being too flexible? Since it wouldn't have any base fields, it means that it isn't strictly a "relationship" entity type, necessarily. Someone could use it to do something completely different. I don't know if that's good or bad. The only control we have is what we call the entity, which is why relationship and metadata both came to mind. The former emphasizes that it's for "relationships" (although that's only a suggestion), the latter that it's for "metadata" (but doesn't necessarily need to have any relationships, technically speaking).

I can't help but also think about the "farm-level data" problem that we've also been discussing on dev calls recently: where do we store data that isn't necessarily tied to any particular asset, including "profile" and "summary" information that you might want to share with third party services? Technically a metadata entity could have a bundle for that. But do we want to use the same entity type for all of these things? Big questions... bigger than I expected from this PR... 😅

[mstenta]

Another question to add to the pile: should we make these entities revisionable? I didn't do that in this simple first pass, but it seems like a logical next step to consider. And the level of effort is low, I think, so if we're going to do it we might as well do it from the start.

[wotnak]

When it comes to the relationship entity, there should probably always be at least two entities involved between which the relationship exists. I was thinking the relationship entity could have two dynamic entity reference base fields that would reference farmOS entities that are in the relationship. Then relationship types (relationship entity bundles) could add more fields, but even without them the relationship would be meaningful (there's a relationship $type between $entity1 and $entity2).

When it comes to general metadata, there could be probably generic metadata entity type with one not required dynamic entity reference 'owner' base field that would be used to assign the metadata to a specific entity and if it would be empty then we could assume the metadata is about the farm in general.

If the metadata entity was added, then we could consider making relationships bundles of metadata entity that would implement an interface that would indicate the metadata describes a relationship. For the relationship metadata entity, the owner base field could then store the entity that introduced the relationship, it could be one of the entities in the relationship or some other entity. For example, birth log could have a relationship in which the log entity would be the owner, first entity would be the mother asset and second entity the child asset.
Or we could keep them separate and allow assigning metadata to relationship entities.

[wotnak]

Using generic relationship and/or metadata entities could make the data model more flexible, more easily extendable. But it also could have a big impact on the current data model. For example, instead of creating new log types for every activity, there could be a single one with some quick forms that would simplify adding relationships/metadata for specific activities and would tag the log with a category that would help to determine the activity type at first glance. It would also make some things like building views with entities that have relationships/metadata assigned more complex.

This may be worth thinking about, but I don't think that this should block this pr. Plan record entities will be useful right away and if in the future generic relationship/metadata entities would be added, the plan record entities could always be migrated to the new data type.

[mstenta]

These are all great ideas/considerations @wotnak! And very true that we can have a deeper discussion about "metadata" entities in the medium/long term while also moving forward with this PR for plans specifically in the short-term. Perhaps farmOS 4.x could include some larger/breaking data model changes using a new entity type as you describe, and this plan_record entity is used in 3.x in the meantime.

[mstenta]

It should also be noted that we can consider experimenting with all of this in contrib, including this new plan_record entity type. If it seems too early to commit to farmOS core, then I can split it out to a contrib module, and farm_crop_plan can depend on that. I don't feel very strongly either way. It's a bit easier in core because we can minimally extend the existing farm_entity module logic to add bundle plugin support to plan_record entities alongside other core types. But that can be done in contrib as well.

[symbioquine]

This is a lot to take in. Just read the comments above so far...

A few thoughts that came to mind:

• Why should this be in an optional module on top of farm_plan? This feels pretty key to most ways plans might be used.
• It took me a while to get used to the naming "plan_record" in this discussion. I wonder if there's a name that would capture the notion of "this is the link between the plan and its constituent plantings/actions/etc better"... Something like "plan_lineitem", "plan_component", "plan_aspect" (idk, just spit-balling here. "plan_record" isn't bad, just doesn't convey much information to make the purpose intuitive either)
• Regarding the concept of normalizing the relationships out into a separate entity, my gut feeling would be to avoid that unless there are requirements for many-to-many relationships or atomicity that can't be met some other way. It sounds like it could easily add a lot of data-model complexity without much benefit.

[mstenta]

Thanks for the review @symbioquine!

Why should this be in an optional module on top of farm_plan? This feels pretty key to most ways plans might be used.

I think that's an open question. There may be many "simple" plans that don't need as much additional information. That said, it's still early and maybe over time a lot of them would end up adopting something like this anyway. I think leaving it optional for now gives them the opportunity, and if we find that it's used more often then not then maybe we make it required in a future release.

It took me a while to get used to the naming "plan_record" in this discussion.

Yea I'll take the blame for that. It was the "least bad" name I could think of. 😅

Worth noting, plan_record is the machine name / entity ID, but in the module itself it is referred to as "Plan record relationship" everywhere as the "human readable" label for the entity type.

I also considered plan_rel, plan_relationship, plan_record_relationship, etc, but ultimately landed on plan_record as the best of all those, trying to find the right balance between intuitiveness and brevity.

Certainly open to renaming it, but I think I still like it the best, out of all the options I've considered.

[mstenta]

Rebased onto latest 3.x (no changes).

[paul121]

If the metadata entity was added, then we could consider making relationships bundles of metadata entity that would implement an interface that would indicate the metadata describes a relationship. For the relationship metadata entity, the owner base field could then store the entity that introduced the relationship, it could be one of the entities in the relationship or some other entity.

@wotnak sounds like Bundle Classes could be useful for what you are describing: https://www.drupal.org/node/3191609

@mstenta after reviewing how the crop plan is using the plan_record entity I was thinking bundle classes could be used there as well. It would be a way to encapsulate the cropPlan service & query/logic helpers directly into the crop_planting bundle of plan_record. eg: $cropPlanService->getPlantingStages($crop_planting) becomes simpler: $crop_planting->getPlantingStages(). With bundle classes each bundle could provide their own class with relevant logic.

The good news is, I don't think we need to do anything in this PR to enable bundle classes. It's something each bundle can opt-in to on their own.

Thinking more generally about plan records being re-used for metadata.... one complexity I might foresee is how this abstract entity type would be used to render data on pages. Are there requirements specific to plan record metadata that would be incompatible with other uses of rendering this metadata? And more specifically, do we plan to view/render this plan_record entity, or even provide editing forms? What would be required along these lines for a more abstract metadata entity type?

Why should this be in an optional module on top of farm_plan?

I thought the same. Seems like this could be included with the core plan module but don't have a strong opinion though.

[mstenta]

Pushed two commits...

The first adds plan to farmOS's entity_reference_integrity config so that it's not possible to delete a plan while there are plan_record entities that reference it.

The second is a patch to entity_reference_integrity to fix a small issue I discovered there: https://www.drupal.org/project/entity_reference_integrity/issues/3418000

[mstenta]

Why should this be in an optional module on top of farm_plan?

I thought the same. Seems like this could be included with the core plan module but don't have a strong opinion though.

I suppose there isn't a strong argument for keeping it in a separate module.

We will need to add a hook_update_N() to plan.install to install the new entity types. I haven't done that before, so I'll need to figure out the right commands... have you ever done that @paul121?

The benefit of a separate module is that's taken care of. But I agree we may as well put it in the same module if we can.

[mstenta]

I haven't done that before, so I'll need to figure out the right commands...

Nevermind we have done that before in the quick form module. :-)

I pushed a commit that merges the plan_record module into the plan module, and adds an update hook to install the new plan_record and plan_record_type entities.

[mstenta]

I think this is ready for review!

[mstenta]

I pushed two more commits to this because I think they are going to be immediately useful in the crop planning module.

The first adds a simple access control handler for plan_record entities which checks access to the referenced plan entity. In other words, if a user can view/edit/delete the plan entity, they can view/edit/delete the plan_record entities that reference it.

The second commit exposes a simple plan_record entity edit form at /plan/record/%/edit. This form hides the plan reference field so that it cannot be modified, but shows everything else. The plan is to use this in an off-canvas dialog in the crop planning module (and others).

[paul121]

One aspect of this new plan_record entity type's data model that I want to highlight is: the plan_record entity reference the plan entity.

Seeing how this is implemented here and used within the crop plan I'm really liking this approach. Part of me would like to wait on merging into core until we can do more testing, but understand it would be nice to get this in soon, and it's probably OK to do so.

I'm curious if we should provide a simple view display for these entities as well, but doesn't need to block this PR.

Also not mentioned above but this also appears in JSON:API and works correctly now with the access controller (viewing that is... I didn't test POSTing but suspect that would work fine.)

[paul121]

Regarding Bundle Logic. I've actually found a nice use-case for this: providing a label for the plan_record entity!

Right now this plan_record entity doesn't have a field to store the label and the default label() method doesn't return anything. The current PlanRecordForm seems to use the UUID as the page title by default. But by providing a bundle class, we can override the label() method to use logic relevant to the each bundle of plan_record. Perhaps we should also provide a simple default implementation of label() for all plan records.

For the Crop Planting bundle that is used in the crop plan we would likely want to incorporate the associated plant asset label, perhaps the associated plan label as well. Here is an example of what this could look like:

namespace Drupal\farm_crop_plan\Bundle;

use Drupal\Core\StringTranslation\TranslatableMarkup;
use Drupal\plan\Entity\PlanRecord;

/**
 * Bundle logic for Crop Planting.
 */
class CropPlanting extends PlanRecord {

  /**
   * {@inheritdoc}
   */
  public function label() {

    // Build label with the referenced plan and plant.
    if ($plan = $this->getPlan()) {
      if ($plant = $this->get('plant')->first()?->entity) {
        return new TranslatableMarkup('Crop planting: %plant - %plan ', ['%plant' => $plant->label(), '%plan' => $plan->label()]);
      }

      // Use the plan if no plant reference.
      return new TranslatableMarkup('Crop Planting - %plan', ['@plan' => $plan->label()]);
    }

    // Fallback to default.
    return parent::label();

}

This label is also used in the entity reference integrity message when preventing access to delete records:

[mstenta]

I love these bundle class ideas too @paul121! Let's experiment with them in downstream code, and maybe it will make sense to adopt them more widely in farmOS core in the near future! I could see them being useful for other entity types too perhaps!

[symbioquine]

LGTM!

[wotnak]

Added two suggestions related to code style/standards, but overall it looks good to be merged.

[wotnak]

use of \Drupal:: calls should be avoided in classes (with exception for static methods) instead dependency injection should be used, although in the case of entity types this isn't currently really possible https://www.drupal.org/project/drupal/issues/2142515
but EntityBase class which ContentEntityBase extends provides entityTypeManager() method, so maybe a good idea would be to use it here instead of directly calling \Drupal:: so when DI capabilities for entity types will be added in core and the EntityBase::entityTypeManager() method will be updated to use it we could automatically take advantage of it without changing farmOS code

Suggested change

	$type = \Drupal::entityTypeManager()
	$type = $this->entityTypeManager()

[mstenta]

Oh good point @wotnak. We actually do this in a few other entity types in farmOS core (most of the time we copy and paste code when creating new entity types). So I might open a separate PR to replace all of them together. Thanks for the heads up!

[mstenta]

I made a commit in a new 3.x-entitytypemanager branch on top of this branch: mstenta@ffb1e8f

I'll open a PR to make that change once this is merged.

[mstenta]

#796

[mstenta]

Tests are passing, we have three approvals, I think this is good to go! I merged the fixup commit. Marking as "ready to merge"!