I recently wrote a test which passes an instance of TranslatableMarkup to \Drupal\commerce_order\AvailabilityResult::unavailable. It failed the assertion:

  public static function unavailable($reason = NULL) : AvailabilityResult {
    assert(is_string($reason) || is_null($reason));

TranslatableMarkup can be converted into a string, but it is not a string.

CommentFileSizeAuthor
#11 3151272-final.patch1.09 KBshubhangi1995
#8 3151272-8.patch1.23 KBshubhangi1995
#4 3151272-4.patch1.21 KBshubhangi1995
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mglaman created an issue. See original summary.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Issue tags: +Novice

The fix is to also support \Drupal\Component\Render\MarkupInterface in the assertion.

  public function __construct($result, $reason = NULL) {
    assert(is_bool($result));
    assert(is_string($reason) || is_null($reason) || $reason instanceof MarkupInterface);

  public static function unavailable($reason = NULL) : AvailabilityResult {
    assert(is_string($reason) || is_null($reason) || $reason instanceof MarkupInterface);
shubhangi1995’s picture

shubhangi1995
Location GURGAON
CreditAttribution: shubhangi1995 commented
Assigned: Unassigned » shubhangi1995
shubhangi1995’s picture

shubhangi1995
Location GURGAON
CreditAttribution: shubhangi1995 commented
FileSize
3151272-4.patch1.21 KB

Please review

shubhangi1995’s picture

shubhangi1995
Location GURGAON
CreditAttribution: shubhangi1995 commented
Assigned: shubhangi1995 » Unassigned
Status: Active » Needs review

Status: Needs review » Needs work

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

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

Thanks, @shubhangi1995. I provided the changes we required in #2. We don't want to convert to a string but change our assertions to allow for a MarkupInterface object.

shubhangi1995’s picture

shubhangi1995
Location GURGAON
CreditAttribution: shubhangi1995 commented
Status: Needs work » Needs review
FileSize
3151272-8.patch1.23 KB

please review.
Just used the changes mentioned in #2

Status: Needs review » Needs work

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

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented 
  1. +++ b/modules/order/src/AvailabilityResult.php
@@ -1,11 +1,12 @@
+use Drupal\Component\Render\MarkupInterface;
...
-final class AvailabilityResult {
+final class AvailabilityResult implements MarkupInterface {

    We don't want to implement the interface. 

  2. +++ b/modules/order/src/AvailabilityResult.php
@@ -31,7 +32,7 @@ final class AvailabilityResult {
-    assert(is_string($reason) || is_null($reason));
+    assert(is_string($reason) || is_null($reason) || $reason instanceof MarkupInterface);

@@ -54,7 +55,7 @@ final class AvailabilityResult {
-    assert(is_string($reason) || is_null($reason));
+    assert(is_string($reason) || is_null($reason) || $reason instanceof MarkupInterface);

    👍 perfect

shubhangi1995’s picture

shubhangi1995
Location GURGAON
CreditAttribution: shubhangi1995 commented
FileSize
3151272-final.patch1.09 KB

sorry and thanks :)

shubhangi1995’s picture

shubhangi1995
Location GURGAON
CreditAttribution: shubhangi1995 commented
Status: Needs work » Needs review
adityasingh’s picture

adityasingh CreditAttribution: adityasingh as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented
Assigned: Unassigned » adityasingh
adityasingh’s picture

adityasingh CreditAttribution: adityasingh as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented
Assigned: adityasingh » Unassigned
Status: Needs review » Reviewed & tested by the community

Patch #11 looks good for me and applied cleanly. I am moving to RTBC.

% Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1116  100  1116    0     0   1388      0 --:--:-- --:--:-- --:--:--  1388
Checking patch modules/order/src/AvailabilityResult.php...
Applied patch modules/order/src/AvailabilityResult.php cleanly.
jsacksick’s picture

jsacksick CreditAttribution: jsacksick at Centarro commented

Btw, we can probably remove the assert() from unavailable() since it calls the constructor which already has it. So doing the same check twice seems a little bit unnecessary.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Status: Reviewed & tested by the community » Fixed

Thanks @shubhangi1995! Commited, 🥳.

Status: Fixed » Closed (fixed)

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