Standardize way to access SKU fields by providing a dedicated EntityWithSkuInterface

Created on 2 September 2024, 15 days ago

As discussed on Slack with @jsacksick and @zaporylie, we should refactor the get/setSku() functions from ProductVariationInterface into a new EntityWithSkuInterface, allowing any custom purchasable entity type to implement this function.

This way, we allow other modules, dealing with order exports, tracking pixel, to have a better, standardized access to the SKU information. Now, these modules often do an instanceof ProductVariationInterrface check, getting the SKU, and then do a fallback to the entity ID. If you implement a custom purchasable entity type in your project, you always have to find a way, to alter this data, to get the SKU of your custom entity type inside

📌 Task
Status

Needs review

Version

3.0

Component

Product

Created by

🇦🇹Austria agoradesign

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @agoradesign
  • Status changed to Needs review 15 days ago
  • Pipeline finished with Success
    15 days ago
    Total: 503s
    #271426
  • 🇮🇱Israel jsacksick

    Generally fine with the change, only thing that's making me hesitant as discussed in Slack is the return types... Just trying to avoid breaking existing code, but that is probably fine because this targets 3.0.x.

    If we make the change, we should as well specify "static" as the return type for the setter. And we need to investigate the test failures.

  • Pipeline finished with Success
    14 days ago
    Total: 575s
    #271516
  • 🇦🇹Austria agoradesign

    It looks quite random, that the tests fail here and not in general:

    Drupal\Tests\commerce_product\Functional\Jsonapi\ProductVariationResourceTest::testIndividual
    The 'errors' member was not as expected.
    Failed asserting that two arrays are equal.
    --- Expected
    +++ Actual
    @@ @@
     Array (
         0 => Array (
    -        'detail' => 'title: Title: this field cannot hold more than 1 values.'
    +        'detail' => 'sku: The SKU "ABC123" is already in use and must be unique.'
             'source' => Array (
    -            'pointer' => '/data/attributes/title'
    +            'pointer' => '/data/attributes/sku'
             )
             'status' => '422'
             'title' => 'Unprocessable Content'
         )
    +    1 => [...]
     )

    That's part of the core's jsonapi base class test testPostIndividual(). Entities with a label key are posted twice, in order to trigger the "field cannot hold more than 1 valoues" error for the title field. As the SKU field has a unique constraint, we also trigger this error. I wonder, why this works for the regular branch, but not for this MR

  • 🇳🇴Norway zaporylie

    I would feel much better if we had any kind of testing included in the MR that would fail if we accidentally introduce regression in the future (change interface name, namespace, signature, etc). I believe https://git.drupalcode.org/project/commerce/-/blob/3.0.x/modules/product... is a good candidate where the interface can be checked. This way if we ever mess up with it, at least we have a asset somewhere in our test suite.

Production build 0.71.5 2024