Do not use an illegal message type

Created on 14 September 2023, 10 months ago
Updated 5 April 2024, 3 months ago

Problem/Motivation

We are not adhering to the Messenger API. We are creating status message using an illegal message type called "commerce_add_to_cart_confirmation". It seems that the reason we are doing is is simply to be able to load our JS when the message is displayed, but this can be achieved in ways that do not violate the API.

There are only three message types supported in MessengerInterface:

  1. TYPE_STATUS
  2. TYPE_WARNING
  3. TYPE_ERROR

When we call the MessengerInterface methods, we should always pass one of these supported types.

Our current implementation leads to unexpected behaviour when using custom or contributed themes which rightfully assume that the message types always are one of the above types. For example themes often have some custom logic for loading the right icon depending on the message type, and this can break if an unexpected type is encountered.

Proposed resolution

A quick fix could be to change our hook_page_attachments() implementation to check for the CSS class of our rendered message:

function commerce_add_to_cart_confirmation_page_attachments(array &$attachments) {
  foreach (\Drupal::messenger()->messagesByType(MessengerInterface::TYPE_STATUS) as $message) {
    if (strpos($message, 'commerce-add-to-cart-confirmation') !== FALSE) {
      $attachments['#attached']['library'][] = 'commerce_add_to_cart_confirmation/commerce_add_to_cart_confirmation';
    }
  }
}

But probably a better solution would be to not use messages in the first place. Most core and contrib themes handle messages in their own unique way. Some might show them in bullet lists, or have their own ways to show messages in animated popups (e.g. Bootstrap). Our JS code blindly yanks the message from the page but leaves the surrounding markup in place, leaving an empty message container behind.

If we would use our own dedicated block type we could reliably show the confirmations, and it would be easier for themers to override them.

🐛 Bug report
Status

Needs review

Version

1.0

Component

Code

Created by

🇧🇬Bulgaria pfrenssen Sofia

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

Merge Requests

Comments & Activities

  • Issue created by @pfrenssen
  • 🇺🇦Ukraine khiminrm

    Maybe it could work if it was fixed in Drupal Core https://www.drupal.org/project/drupal/issues/3100473

  • 🇺🇦Ukraine khiminrm

    Created MR with solution to separate the add to cart confirmation message from other messages before render.

  • Pipeline finished with Success
    3 months ago
    Total: 166s
    #136517
  • Status changed to Needs review 3 months ago
  • 🇺🇦Ukraine khiminrm

    Maybe another approach we could use - save message in private storage and render it in custom block?

  • 🇮🇱Israel jsacksick

    Private storage is a good idea, but I wouldn't save the whole HTML there, I'd simply save the data that I need to render the message, so just the order item ID?
    Then whenever the private storage is accessed, you can just load the order item, and render the theme function?
    But yeah, using the messenger is kind of hackish (that is how it was done in D7 as well as far as I remember).

  • Pipeline finished with Success
    3 months ago
    Total: 149s
    #137230
  • First commit to issue fork.
  • Pipeline finished with Success
    3 months ago
    Total: 143s
    #137235
  • Merge request !15Resolve #3387351 "Use private storage" → (Open) created by khiminrm
  • Pipeline finished with Success
    3 months ago
    Total: 159s
    #138503
  • 🇺🇦Ukraine khiminrm

    I've created another on MR with the solution to use the private store https://git.drupalcode.org/project/commerce_add_to_cart_confirmation/-/m...

  • Pipeline finished with Success
    3 months ago
    Total: 149s
    #138509
  • 🇮🇱Israel jsacksick

    hm... Helper is a weird name, maybe too generic... And why would you need to have a method to delete the order item ID?
    Also, surprised it isn't the case already, but we night need to store the quantity that was added as well in case somebody decides to not use Views for the cart confirmation.
    Also the MR has code commented out.
    We can maybe keep the code simple, just store directly into the private temp store...Once you introduce an interface then you have to maintain it and you can no longer break BC so you have to think carefully :p.

  • 🇺🇦Ukraine khiminrm

    @jsacksick, I used delete method to delete stored ID and do not show the same message multiple times.
    Could you, please, explain this:
    We can maybe keep the code simple, just store directly into the private temp store

    We shouldn't use additional service and just store value in the event subscriber, right?

  • 🇮🇱Israel jsacksick

    Introducing a new service is not bad idea in general, the problem is that with the current methods defined, there's no room for additional arguments in the future.
    Let's say you'd like to store the quantity that was added, or even the order ID, then your methods are suddenly obsolete.
    Maybe you can go with:

    CartConfirmationManager as the service name?
    Then define the following methods:

    • recordAddToCart(), from here you can accept an array (quantity, order_item_id)...
    • getCartItemInfo()
    • clear()
  • 🇺🇦Ukraine khiminrm

    @jsacksick thanks for your help! I've improved code. Please, review.

  • Pipeline finished with Success
    3 months ago
    Total: 148s
    #138709
  • 🇮🇱Israel jsacksick

    Reviewed, what was the problem with page attachments again?

  • Pipeline finished with Success
    3 months ago
    Total: 151s
    #138742
  • Pipeline finished with Canceled
    3 months ago
    Total: 60s
    #138749
  • Pipeline finished with Success
    3 months ago
    Total: 167s
    #138750
  • 🇺🇦Ukraine khiminrm

    @jsacksick regarding page attachments alter. When I tested there were caching problem. So there were no dialog until clearing a site cache. I can re-test

  • 🇮🇱Israel jsacksick

    hm... You might miss cache context / tags then perhaps...

  • 🇺🇦Ukraine khiminrm

    re-tested. when using code inside commerce_add_to_cart_confirmation_page_attachments() it doesn't work - need to clear cache to see the dialog.

  • Pipeline finished with Success
    3 months ago
    Total: 147s
    #140917
Production build 0.69.0 2024