Make the AvailabilityManager order aware

Going to review

We should be able to utilize @trigger_error and tell our tests to expect that deprecation.

Should move this to #2937041: AvailabilityManager needs to be aware of the order, if there is one instead of a new issue. Or I guess that is not really needed.

We shouldn't title this "Deprecate the AvailabilityManager", it's more "Make the AvailabilityManager order aware", which causes moving it to the order module and deprecating the old service. Which is why, with that name, I thought we could just use the existing issue which has 14 followers.

We can hack on it here and then decide what to do with the related issues.

We should probably close #3107547: Availability checks should use a value object instead of boolean return values for this. There's no reason to land the availability result object without a new method to utilize it, especially if we plan on rewriting the entire tidbit.

Initial review, (I just realized this was on the interdiff) will follow up with more.

1. +++ b/src/AvailabilityCheckerInterface.php
   @@ -2,10 +2,10 @@
   -@trigger_error('The ' . __NAMESPACE__ . '\AvailabilityCheckerInterface is deprecated. Instead, use \Drupal\commerce_order\AvailabilityCheckerInterface', E_USER_DEPRECATED);
   -
   

   I think it's fine to abandon this interface and keep the @trigger_error, since the interface is technically dead code for us, internally.

2. +++ b/src/AvailabilityManager.php
   @@ -2,10 +2,10 @@
   -@trigger_error('The ' . __NAMESPACE__ . '\AvailabilityManager is deprecated. Instead, use \Drupal\commerce_order\AvailabilityManagerInterface', E_USER_DEPRECATED);
   -
   

   If this is causing grief, because of the trigger error at the top of the screen, what if we called the trigger error during check?

   That's what I did in #3107547-21: Availability checks should use a value object instead of boolean return values when we receive a truthy value.

   That's what core did for EntityManager: https://git.drupalcode.org/project/drupal/-/blob/8.8.x/core/lib/Drupal/C.... Granted it was deprecated over time, but we can leverage this.

   If someone is calling the old service ::check they'll get the deprecation.

3. +++ b/src/AvailabilityManager.php
   @@ -2,10 +2,10 @@
   + * @deprecated Use \Drupal\commerce_order\AvailabilityManager instead.
   

   This needs to have the messaging "Depredated in X will be removed in X use X"

4. +++ b/modules/order/src/AvailabilityManager.php
   @@ -0,0 +1,92 @@
   +    assert($checker instanceof AvailabilityCheckerInterface);
   ...
   +    assert($checker instanceof LegacyAvailabilityCheckerInterface);
   

   Assert isn't needed, the code would crash since we have a typed parameter.

5. +++ b/modules/order/src/AvailabilityManager.php
   @@ -0,0 +1,92 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCheckers() {
   +    return $this->checkers;
   +  }
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getLegacyCheckers() {
   +    return $this->legacyCheckers;
   +  }
   
   +++ b/modules/order/src/AvailabilityManagerInterface.php
   @@ -0,0 +1,69 @@
   +  /**
   +   * Gets all added the checkers.
   +   *
   +   * @return \Drupal\commerce_order\AvailabilityCheckerInterface[]
   +   *   The checkers.
   +   */
   +  public function getCheckers();
   ...
   +  /**
   +   * Gets all added the "legacy" checkers.
   +   *
   +   * @return \Drupal\commerce\AvailabilityCheckerInterface[]
   +   *   The checkers.
   +   */
   +  public function getLegacyCheckers();
   

   Do we need to allow getting them? it should just be about check

6. +++ b/modules/order/src/AvailabilityManager.php
   @@ -0,0 +1,92 @@
   +      $result = $checker->check($order_item, $context);
   +      if (empty($result)) {
   

   Should we allow an empty result? Should you be forced to return available? And if anything, I'd prefer changing not empty to a direct instance check

7. +++ b/modules/order/src/AvailabilityManager.php
   @@ -0,0 +1,92 @@
   +    foreach ($this->legacyCheckers as $checker) {
   +      if (!$checker->applies($purchased_entity)) {
   +        continue;
   +      }
   +      $result = $checker->check($purchased_entity, $quantity, $context);
   +      if ($result === FALSE) {
   +        return AvailabilityResult::unavailable();
   

   This is where we can throw trigger errors for deprecated implementations, like in #3107547-21: Availability checks should use a value object instead of boolean return values.

   Before we even check if it applies, trigger a deprecation that we're checking a legacy checker.

8. +++ b/modules/order/src/AvailabilityOrderProcessor.php
   @@ -20,23 +19,23 @@ class AvailabilityOrderProcessor implements OrderProcessorInterface {
   -    $this->orderItemStorage = $entity_type_manager->getStorage('commerce_order_item');
   +    $this->entityTypeManager = $entity_type_manager;
   
   @@ -63,11 +62,12 @@ class AvailabilityOrderProcessor implements OrderProcessorInterface {
   +    $order_item_storage = $this->entityTypeManager->getStorage('commerce_order_item');
   

   👍

9. +++ b/modules/order/src/AvailabilityResult.php
   @@ -0,0 +1,91 @@
   +  /**
   +   * The availability result.
   +   *
   +   * @var bool|null
   +   */
   +  protected $result;
   ...
   +   * @param bool $result
   +   *   (optional) The availability result (FALSE is unavailable).
   ...
   +  public function __construct($result = NULL, $reason = NULL) {
   

   It should default to FALSE, shouldn't it? Why should we allow NULL when it's either TRUE or FALSE.

10. +++ b/modules/order/src/AvailabilityResult.php
    @@ -0,0 +1,91 @@
    +  public static function neutral() : AvailabilityResult {
    +    return new static();
    +  }
    

    This should return TRUE. The access system supports neutral in the fact if nothing says allowed, access is denied. Neutral means pass. Our system is "if nothing said unavailable, it is available"

    It also shows in the fact we don't have a matching "available" which sets TRUE, which is perfect. Let's just have this explicitly set TRUE and make $result not be nillable

11. +++ b/tests/modules/commerce_test/commerce_test.services.yml
    diff --git a/tests/modules/commerce_test/src/TestAvailabilityChecker.php b/tests/modules/commerce_test/src/TestAvailabilityChecker.php
    deleted file mode 100644
    
    deleted file mode 100644
    index 34fdbae4..00000000
    
    index 34fdbae4..00000000
    --- a/tests/modules/commerce_test/src/TestAvailabilityChecker.php
    

    Should we keep this to prove we support legacy checkers? Our tests can silence deprecations, core did this.

12. I didn't apply locally yet, but I saw no changes for tests/src/Unit/AvailabilityManagerTest.php, per the last item, is this our test that the legacy checkers still work?

This patch looks great!

@agoradesign had a good item for feedback that would introduce the explicit ::available method.

product XY should always be sold, even if it's out of stock. of course, you can put this logic into the availability manager that handles all of your products, but you may want to introduce a XyAvailabilityChecker, which applies only to XY and wants to return "yes, it's always open for sale".

So you could set XyAvailabilityChecker at the highest priority to explicitly allow specific SKUs.

The alternative is you craft proper logic in the applies method. My example is if a site supports dropshipping. Your normal availability checker for inventory should see if field_is_dropshipped is true, and not apply to the order item.

The code is pretty simple right now. Adding ::available adds more logic and possibly accidents. I wanted to at least capture this here as notes. I think we can commit this and still add an ::available method without worrying about backward compatibility for explicit "always allow"

FINALLY ... after years ...

;) Guys, this is cool stuff. Unfortunately I can't provide the time for a line by line review this week, but the concept is clean and simple and the patch looks well coded. I'm really happy to see this happen. It will give us the API we need to remove a bunch of custom form alter hooks and stuff in commerce_stock. Allows for much cleaner code and testing.

#12 We have a "AlwaysAvailable" stock service in commerce stock from the beginning, exactly for such use cases. We will see how it goes in practice, as soon as we update our code to the new AvailablityManager API. I think these use cases are the more advanced ones. In such projects/scenarios there should be a developer nearby, that can code a proper applies logic to your custom AvailabilityManager. And as you said, if we miss something, we can add it later.

Thanks for the work!

Working on the change record

Wrote the change record. Getting peer review of the text, and then committing.

🥳 Committed! Thanks @jsacksick, and @olafkarsten for the input.