traits should be installed at the API level, not the form level

Had a look at this issue as part of getting into contributing.

Attached is a new patch, hopfully this was the intention of a cleanup. The `submitTraitForm` from `CommerceBundleEntityFormBase.php` has been removed, as well as the calls in the extending Forms.

Let's change the target branch... Could you please open an MR to see if the tests are passing? I wonder if we have tests coverage for trait installation / uninstallation.

Merge request created

Tests are failing, setting the status to "Needs work".

The test that is failing is the 'testDuplicateTraits' test of 'Drupal\Tests\commerce\Functional\EntityTraitTest'.

When an entity type is cloned from an entity type with an EntityTrait that provides a field, the test checks that by removing the EntityTrait and saving the new cloned entity type the provided field is also removed. As the entity being saved returns TRUE when it's 'isNew()' method is called, the save logic assumes that there are no original traits.

    if ($this->isNew()) {
      $original_traits = [];
    } else {
      $original = $this->entityTypeManager()->getStorage($this->getEntityTypeId())->loadUnchanged($this->id());
      $original_traits = $original->getTraits();
    }

When the trait handling logic was handled at the form level, it was called after the entity had been saved, so at that point the entity was no longer new and the original traits from the cloned entity were loaded.

I feel like we should either:
A) Not go ahead with this change now that we know why the traits are handled at the form level and not the API level. Perhaps adding a comment to explain why.
B) Not assume that just because an entity is new it has no traits as this does not account for the situation where the new entity has been cloned from an entity that has traits.

I hope this is helpful and not poorly explained. I'll be interested to hear your thoughts.

hm... What about using postSave() then?

Moving the logic to the 'postSave' has its own issues due to the way that the 'selected_traits' and 'unselected_traits' are calculated.

    $selected_traits = array_diff($traits, $original_traits);
    $unselected_traits = array_diff($original_traits, $traits);

With this inside of the 'postSave', there is no longer visability of the original traits on the entity (unless I'm missing something) in order to work out what traits have been added and removed as the new selection has already been saved to the entity.

And do you know why we aren't calling the parent presave() method first? Rather than accessing the original traits prior to that?

Considering the small interest expressed with this, wondering if we should really spend time "fixing" this.

Having a further look, the fields from the source entity are coppied accross to the cloned entity in the 'postSave' of 'EntityDuplicateFormTrait', which is called after the entity save.

Even if in the entity save we handle the added / removed traits' fields, they are added again afterward in the 'SourceTypeForm'. I think that this is why the trait logic is handled at the form level, so that it can be the last word after both the entity saves and 'EntityDuplicateFormTrait'.

I think that there would have been value in making it clearer as to why it's done in this order. Do you think that it would be good to add a comment to explain why it has been done this way?

Considering the small interest expressed with this, wondering if we should really spend time "fixing" this.

Personally, I've been looking at this issue as part of my efforts to start getting into making contributions, so I've been spending time on it exactly because it's not high priority :)

I think that there would have been value in making it clearer as to why it's done in this order. Do you think that it would be good to add a comment to explain why it has been done this way?

Definitely!

daniel.j → closed merge request !347

This means that creating and saving an entity through the API, as you would in a test, doesn't add trait fields

Looking back at the issue description, I think that the issue is still valid. However, the postSave method of EntityDuplicateFormTrait (which dispatches an event listened to by EntityDuplicateSubscriber) would need to not re-add fields that have been removed in the entity save or postSave as part of the removal of an EntityTrait.