We need to show a message and disable all fields on the package type edit form, if there's a shipment entity that specifies the plugin derived from that package type config entity. We'll want a helper method inside the form class that will do an entity query.

CommentFileSizeAuthor
#4 commerce_shipping-admin-ui-initial-2854035-4.patch4.25 KBjsacksick
#3 commerce_shipping-prevent-deleting-already-in-use-package-type-2841471-3.patch4.22 KBjsacksick
#2 commerce_shipping-prevent-deleting-already-in-use-package-type-2841471-2.patch3.09 KBjsacksick
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz created an issue. See original summary.

jsacksick’s picture

jsacksick CreditAttribution: jsacksick at Centarro commented
Status: Active » Needs review
FileSize
commerce_shipping-prevent-deleting-already-in-use-package-type-2841471-2.patch3.09 KB

I figured we probably need to implement an access control handler for the package type to prevent deleting already in use package types.
Should we extend the PackageTypeTest?
Let me know if the message needs to be polished as well.

jsacksick’s picture

jsacksick CreditAttribution: jsacksick at Centarro commented
FileSize
commerce_shipping-prevent-deleting-already-in-use-package-type-2841471-3.patch4.22 KB

Forgot the Access control handler.

jsacksick’s picture

jsacksick CreditAttribution: jsacksick at Centarro commented
FileSize
commerce_shipping-admin-ui-initial-2854035-4.patch4.25 KB 
-    $query = \Drupal::entityQuery('commerce_shipment');
+    $query = $this->entityTypeManager()->getStorage('commerce_shipment')->getQuery();

I now get the query from the storage.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

We want to generalize this concept to Commerce itself.

However, the current approach is potentially too expensive. An admin listing with 50 entities means 50 calls to access('delete'), which calls isUsed(), which means at least 50 entity queries (sometimes more).

We could call isUsed() from the delete form instead, then again from the controller as a protection against API deletes (REST or just code calling $entity->delete()), but the question then becomes whether we should skip deletion silently, set a message, or throw an exception.

skyredwang’s picture

skyredwang
Location Shanghai
CreditAttribution: skyredwang at INsReady commented

@bojanz sent me here from IRC, because I have a production DC site, currently with 469 colors, 18 sizes and 538 styles. They are all product attributes. I haven't tested the patch yet, because I am not using shipping. I just want to provide an example that expensive checking won't work well for me in general.

I will revisit this discussion once I got more time.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

This is what EntityResource (REST) does:

  public function delete(EntityInterface $entity) {
    $entity_access = $entity->access('delete', NULL, TRUE);
    if (!$entity_access->isAllowed()) {
      throw new AccessDeniedHttpException($entity_access->getReason() ?: $this->generateFallbackAccessDeniedMessage($entity, 'delete'));
    }

So, to be able to show why deletion is forbidden, we either need to use entity access, or to override this class and method to perform our separate isUsed check as well.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter(), +#2824572: Write EntityResourceTestBase subclasses for every other entity type.

The problem in this issue is: how to efficiently list N entities, and the access check for each entity needing an entity query over M entities.

This is a known problem. You could solve this with the Node Access API — hook_node_grants()'s grant_delete allows you to efficiently query. Unfortunately that only works for nodes, and we're obviously not dealing with nodes here. #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() exists to generalize that API to all entity types. That issue would help you a lot.

#5:

An admin listing with 50 entities means 50 calls to access('delete'), which calls isUsed(), which means at least 50 entity queries (sometimes more).

Note that Views have row-level caching, and access results have cacheability metadata. So, the access result would have the commerce_shipment_list cache tag. Which would mean that you'd only re-run that access check if any of the M entities had changed. Now, I can easily see shipment entities changing frequently, so that doesn't actually help you.
You could create a custom cache tag, something like commerce_shipment_using_package_type_{TYPE}_list, add some logic to Shipment::preSave() that invalidates this cache tag when appropriate (i.e. when a shipment is created that uses this package type, or when the package type is changed and either the pre or post value matches this package type).

#6: if you're not using shipments, then how is your site affected? Is it because colors/sizes/styles also affect whether package types can be deleted or not?

#7: I'm not sure what you're asking there. 3 possible interpretations, 3 answers:

  1. The way REST does access checking is irrelevant here, because REST always operates on single entities.
  2. The code you showed is indeed using entity access, but so is the patch in #4, so REST will actually respect this new access checking just fine.
  3. If you're asking how to show a reason for denying access: the current patch does return AccessResult::forbidden()->addCacheableDependency($entity);, you can simply change that to return AccessResult::forbidden('This package type cannot be deleted because shipments are using it.')->addCacheableDependency($entity);, and the reason

The patch in #4 looks good to me. Thoughts:

  1. You definitely should add explicit test coverage for the new access control handler that's being added.
  2. You could add the cache tag-based optimization I mentioned.
  3. You could add explicit test coverage to ensure that your entity listings indeed benefit from this cache tag-based optimization, to ensure good performance.
  4. You really should add REST test coverage. We made that easy with \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase — I know @tstoeckler was very enthusiastic about how it helped him add REST test coverage for his custom entity types. See #2824572: Write EntityResourceTestBase subclasses for every other entity type. for lots and lots of examples.
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

However, the current approach is potentially too expensive. An admin listing with 50 entities means 50 calls to access('delete'), which calls isUsed(), which means at least 50 entity queries (sometimes more).

There is are quite some conditions here, I'm curious have you done some profiling whether this naive approach is too slow? I would guess the slowness is mostly focused around the entity query. Given that I'm curious whether you experienced with prefetching the entity query. Wouldn't that be possible using some of our thousand hooks and places to interact?