Support from Acquia helps fund testing for Drupal Acquia logo

Comments

powysm created an issue. See original summary.

bojanz’s picture

Category: Bug report » Support request
Status: Active » Fixed

No it's not, that's set by bundleFieldDefinitions(). Same trick used by the Comment entity in core.

powysm’s picture

Ah. I am having alot of problems with line items entity reference relationship with purchased entity when using views. It is being referenced as 'content' rather than the specific entity type, which means I get a bunch of fields but not product variation specific (sku etc.)? I thought that the above may have been the issue?

Just tried a similar simple relationship between nodes and comments and not getting the same issue.

bojanz’s picture

Title: LineItem 'purchased_entity' reference missing target_type » Views has problems using the purchased_entity field
Category: Support request » Bug report
Status: Fixed » Active
Issue tags: -lineitem, -puchased, -entity, -target

I see.

I'll review our Views integration. Maybe we're missing a trick that core has for Comment. Or running into a bug.

bojanz’s picture

Confirmed that we're missing code. Comment defines a \Drupal\comment\CommentViewsData class which has the following:

    // Provide a relationship for each entity type except comment.
    foreach ($entities_types as $type => $entity_type) {
      if ($type == 'comment' || !$entity_type->isSubclassOf('\Drupal\Core\Entity\ContentEntityInterface') || !$entity_type->getBaseTable()) {
        continue;
      }
      if ($fields = \Drupal::service('comment.manager')->getFields($type)) {
        $data['comment_field_data'][$type] = array(
          'relationship' => array(
            'title' => $entity_type->getLabel(),
            'help' => t('The @entity_type to which the comment is a reply to.', array('@entity_type' => $entity_type->getLabel())),
            'base' => $entity_type->getDataTable() ?: $entity_type->getBaseTable(),
            'base field' => $entity_type->getKey('id'),
            'relationship field' => 'entity_id',
            'id' => 'standard',
            'label' => $entity_type->getLabel(),
            'extra' => array(
              array(
                'field' => 'entity_type',
                'value' => $type,
                'table' => 'comment_field_data'
              ),
            ),
          ),
        );
      }
    }

We'll need something similar, while making sure that the default relationship to node is not available.

(Note that there's also some code in comment_views_data_alter() which appears unneeded.)

vasike’s picture

Here is a patch that implements purchased entity relationships for all target entities types of the line item purchased entity fields defined.

vasike’s picture

there is an update for patch, trying to improve the code for getting the entities types for the relationships

bojanz’s picture

Status: Needs review » Fixed

Tweaked and committed. Thanks!

  • bojanz committed 481a11e on 8.x-2.x authored by vasike
    Issue #2638958 by vasike: Views has problems using the purchased_entity...

Status: Fixed » Closed (fixed)

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

niko-’s picture

Priority: Normal » Major

Hi @all

purchased_entity field has view modes from node entity as target_type not seted so node is used as default.
Maybe

->setSetting('target_type', 'commerce_product_variation')

have sanse?

andypost’s picture

This needs followup to add "extra" section to relationship to make entity reference to know entity type to use ER field renderer

bojanz’s picture

Status: Closed (fixed) » Needs work

Merging back #2840386: Views assume that purchased_entity is node. into this issue and reopening.

Turns out that the relationship added by this issue doesn't work, we are missing the "extra" join information as andypost points out. However, we can't add it cause we don't actually store the purchased_entity_type that we'd need for that join to work.
Fixing the relationship would require adding a purchased_entity_type field and populating it in an update hook.

However, I am not convinced we should be allowing people to use this relationship. Our canonical way of showing purchased entity information has been to render the entity (product variation, bundle, etc) in the first column. This works for carts that have different purchased entity types (mixture of regular products, bundles, etc), unlike the relationships.

This brings us back to #2840386: Views assume that purchased_entity is node., which showed us problems with rendering the purchased entity. Due to the way Views uses fields (storage-only, no bundle-level overrides) it doesn't detect which entity type is being referenced, falling back to node.

I propose the following fix for now:
1) Remove the relationship code, document that the purchased_entity should be rendered instead.
2) Modify OrderItem::baseFieldDefinitions() and set the purchased_entity target type to "commerce_product_variation".
This is a better default than "node" since it's at least true in some cases.
That means that when configuring the rendered entity Views will always offer the product variation view modes, but that's fine since the matching view modes should exist for other puchasable entity types as well.

niko-’s picture

jespermb’s picture

Hi, we tried the patch but when installing the site we get this error:

exception 'Drupal\Component\Plugin\Exception\PluginNotFoundException' with message 'The "commerce_product_variation" entity type does not exist.' in /mnt/www/html/interflora/docroot/core/lib/Drupal/Core/Entity/EntityTypeManager.php:133

// Jesper

jagundez’s picture

Same problem, the patches doesn't work for me :(

bradjones1’s picture

I'm not entirely sure the use-cases driving all of the above, however I do have a concern around hard-coding any assumptions around the target type for the purchased_entity field, because it's not necessarily a product variation. In my case, I have an entity type implementing PurchaseableEntityInterface and need to access it via that field.

Attached is a patch I worked up on the same code being altered in these patches, which adds a back-relationship for order items referencing a purchased entity. It might not necessarily solve everyone's concerns that led them to this issue, but I would vote against any architectural bias toward the product variation being the only type of entity you may get from this reference.

bojanz’s picture

@bradjones1
Please reread my comment. There is no fix to the Views integration that we can make without introducing another order item field and updating every order item in existence.

The assumptions we're making for the fix are this:
1) People will always render the purchased entity to show it in the cart
2) All purchasable entity types have the same required view modes
Those assumptions allow us to tell Views to assume commerce_product_variation without breaking other purchasable entity types.

bradjones1’s picture

@bojanz

Actually I think my patch addresses a different use case entirely, I was probably distracted by the fact the patches on this issue affect the same codebase. Re: showing the purchased entity, I agree about the limitations and have run into this myself in the cart review view.

My patch, which might need to be split into a different issue after all, provides a purchased entity -> order item reference in views, which I think is another case entirely? I'm for instance using it to provide a view of a user's (custom entity type) purchaseable entities which are related to orders in fulfillment status.

Views is just a tool, of course, and I could create a custom controller to do that query and output similar data, but the opposite-direction relationship I'm providing I think is helpful, since in that case you know you're getting order item(s) always.

edaa’s picture

Same problem in my tests, the 'target_type' always fallback to 'user'.

websiteworkspace’s picture

This appears to be a gnarly problem.

mglaman’s picture

Status: Needs review » Needs work

#17 belongs in its own issue.

I'm struggling to understand the bug. This seems to work fine when rendering purchased entities? We are rendering purchased entities just fine. We have variations and a custom "addon" purchased entity. I am able to render view modes in views without issue. No relationships needed or anything.

I say we close this so we can open a more succinct issue. This original issue added a relationship which works. The purchased_entity field alone with rendered entity/label formatter works.

Per #2

Same problem in my tests, the 'target_type' always fallback to 'user'.

Can you explain? Is that on the base field? Or something in Views?

edaa’s picture

#22

Can you explain? Is that on the base field? Or something in Views?

It is on base field, but the issue is gone now (Commerce 2.2), I don't check if the issue happens on 2.1.

bradjones1’s picture

PlayfulWolf’s picture

FileSize
873.64 KB
70.21 KB

I think the problem is that the view commerce_cart_form needs some rework to be usable, as in default configuration had no luck to display rendered variation properly. One extra relationship is enough, another is for wrapping all the cell in link to product.

p.s. commerce 2.17

rszrama’s picture

Category: Bug report » Task
Priority: Major » Normal
FileSize
2.62 KB

Resurrecting this issue and fixing up the metadata.

This is a failure on core's part, not our own, so I'm making this a normal task. The basic idea is that the core display formatter used here is drawing the target_type from the base field definition irrespective of any bundle specific settings. I don't know where to begin fixing that in core, but bojanz's suggestion in #13 to set a sensible default in our base field definition is the best solution I see.

However, I also think bradjones1's concern about hard-coding a specific dependency on commerce_product_variation is reasonable. We can define a new API for determining a default purchasable entity type from the available types on a site and direct people to alter the base field info if they need something other than our default choice. That's all in the attached patch.

All that said ... discussing this with Matt, it makes sense for Commerce 2.x to have a higher level API, since I basically duplicated code in the OrderItemTypeForm class. We've discussed a PurchasableEntityTypeRepository service with functions that include getAllTypes(), getDefaultType(), and getTypeOptions() that can be used both in OrderItem and OrderItemTypeForm (and elsewhere as need be). I'm going to open a separate issue for that and then we can revise the attached patch once we have that API to work with to resolve this one in similar fashion.

rszrama’s picture

Component: Line item » Order
Status: Needs work » Needs review
FileSize
1.1 KB

The attached patch depends on #3146451: Add a PurchasableEntityTypeRepository service, so I'm leaving this as not tested by testbot. It manually tested locally fine, but we'll wanna let test bot ensure the new setting doesn't cause some weird unexpected conflict elsewhere.

The work in this patch is obviously much simpler than in #26, and the docblock from my previous patch is represented in the new service's interface.

mglaman’s picture

Title: Views has problems using the purchased_entity field » [PP-1] Views has problems using the purchased_entity field
Status: Needs review » Postponed

Marking issue postponed for now, as we sort out #3146451: Add a PurchasableEntityTypeRepository service

mglaman’s picture

Title: [PP-1] Views has problems using the purchased_entity field » Views has problems using the purchased_entity field
Status: Postponed » Needs review

Blocker merged!

Status: Needs review » Needs work

The last submitted patch, 27: 2638958-27.default_purchasable_entity.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mglaman’s picture

This failed in tests where there is no default purchasable entity type available.

EDIT: It happened here, too:

1) Drupal\Tests\commerce_cart\FunctionalJavascript\MultipleCartMultipleVariationTypesTest::testAddToCart
Error: Call to a member function id() on boolean

I'm going to assume it's because Order is higher in alphanumeric priority than Product, so the order fields are built before any known purchasable entity type is available.

mglaman’s picture

Status: Needs work » Needs review
FileSize
1.91 KB
1.02 KB

This should fix things up. The target_type is only set if there is a purchasable entity type available.

rszrama’s picture

Status: Needs review » Reviewed & tested by the community

Tested locally after updating to -dev to get the related fix, and this is workin' good for me! \o/

  • mglaman committed ccf842c on 8.x-2.x authored by rszrama
    Issue #2638958 by rszrama, vasike, mglaman, bradjones1, niko-,...
mglaman’s picture

Status: Reviewed & tested by the community » Fixed

🥳 Committed, take that Views!

Status: Fixed » Closed (fixed)

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