Messenger creates a session for storing flash messages even when response format isn't HTML/it will never show

Created on 11 July 2023, 11 months ago
Updated 21 May 2024, 26 days ago

Problem/Motivation

If code is called which calls the Messenger service to add a message (e.g., a status update when adding a commerce product to a cart), a session will be written to hold the message data. This is fine in browser contexts, however if not using cookie auth (or, really, not using the html format) you get a session for a message that will never display. This can create WTF conditions when testing non-HTML endpoints (e.g., JSON:API) in clients like Insomnia or Postman, and also calls the cache kill switch unnecessarily.

Steps to reproduce

Make a non-HTML request (e.g., when using Bearer token auth) to a page that calls the messenger. Even though you do not need a cookie session, one is created.

Proposed resolution

No-op on setting messages when the request is not html format. We could in theory also consult the auth method, but I think this is unnecessary mixing of concerns.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
System 

Last updated 2 days ago

No maintainer
Created by

🇺🇸United States bradjones1 Digital Nomad Life

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @bradjones1
  • last update 11 months ago
    29,808 pass
  • 🇺🇸United States bradjones1 Digital Nomad Life
  • Status changed to Needs work 11 months ago
  • 🇺🇸United States bradjones1 Digital Nomad Life
  • 🇮🇱Israel jsacksick

    I actually went ahead and applied this changed to one of the projects I'm maintaining and wanted to report a regression I just noticed.
    We're using the VBO export module to generate CSV exports, and the download link is no longer output because of this change, guessing because the BATCH API is using Ajax.

    Additionally, note that the following:

    $this->requestStack->getCurrentRequest() can potentially return NULL.

    Unfortunately, while this change is appealing, I don't think it can be as simple as the fix suggested in the MR.

  • 🇺🇸United States bradjones1 Digital Nomad Life

    $this->requestStack->getCurrentRequest() can potentially return NULL.

    Unfortunately, while this change is appealing, I don't think it can be as simple as the fix suggested in the MR.

    Thanks Jonathan for taking a look.

    I think the null-safety for the current request is easy enough to fix - we could just no-op and keep the current behavior.

    As for AJAX, I had forgotten about that (pretty much all my traffic is decoupled) but it is very much a thing - I think the test could be request format html OR ajax? That would keep this pretty tight?

  • Pipeline finished with Failed
    26 days ago
    Total: 639s
    #177607
  • 🇺🇸United States bradjones1 Digital Nomad Life

    Updated this to also consult the "wrapper format" for the no-op to address #5.

  • Pipeline finished with Success
    26 days ago
    Total: 594s
    #177636
Production build 0.69.0 2024