Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUGFIX: Forward status codes and headers set in module actions #5179

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

Sebobo
Copy link
Member

@Sebobo Sebobo commented Jul 8, 2024

Previously the status code set in module actions via throwStatus or response->setStatusCode() was ignored. With this change the individual module response is merged into the generic module response which then contains the desired status code, headers and other properties.

This is essential if the client (f.e. HTMX) relies on proper headers and status codes to behave properly.

Review instructions

Use f.e. $this->throwStatus(404) in a Neos module controller action and check the status code of the response when calling the action.
The response code should now be 404 instead of 200.

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

Previously the status code set in module actions via `throwStatus` or `response->setStatusCode()` was ignored.
With this change the individual module response is merged into the generic module response
which then contains the desired status code and other properties.
@Sebobo Sebobo changed the title BUGFIX: Return status code set in module controllers instead of 200 BUGFIX: Return status code set in module actions instead of always returning 200 Jul 8, 2024
@Sebobo Sebobo changed the title BUGFIX: Return status code set in module actions instead of always returning 200 BUGFIX: Use status code set in module actions Jul 8, 2024
@Sebobo Sebobo changed the title BUGFIX: Use status code set in module actions BUGFIX: Forward status codes and headers set in module actions Jul 8, 2024
@Sebobo Sebobo requested review from bwaidelich and ahaeslich July 8, 2024 15:26
@Sebobo
Copy link
Member Author

Sebobo commented Jul 10, 2024

@bwaidelich I think HTMX is really hard to use without this fix. I now spent some time to find a good way to create responses to async actions and the usage of Exceptions as we discussed is not really working well.

I'm now using the following function in the controller together with this fix. It send the flash messages via a HTMX header to trigger a custom event in the module which uses the backend API to show the flash messages. HTMX decides with its config whether to swap based on the status code. Though the swap can be controlled with the HX-Reswap header.

    /**
     * This method should be used to return the stored flash messages via HTMX trigger header
     * and set the given status code for HTMX to use.
     */
    protected function sendHTMXResponse(int $statusCode = 200): void
    {
        try {
            $encodedFlashMessages = json_encode([
                'app:notify' => array_map(static function (Message $message) {
                    return [
                        'severity' => $message->getSeverity(),
                        'message' => $message->getMessage(),
                        'title' => $message->getTitle(),
                    ];
                },
                    $this->controllerContext->getFlashMessageContainer()->getMessagesAndFlush(),
                )
            ],
                JSON_THROW_ON_ERROR
            );
        } catch (\JsonException) {
            $encodedFlashMessages = '[]';
        }

        $this->response->setHttpHeader('HX-Trigger', $encodedFlashMessages);
        $this->response->setStatusCode($statusCode);
    }

@Sebobo Sebobo requested a review from mhsdesign July 10, 2024 07:00
@bwaidelich
Copy link
Member

TL;DR I'm fine with the change (if someone else verifies) ;)

I think HTMX is really hard to use without this fix

I see, but since the fix came from me, I don't feel comfortable reviewing it :)

I'm now using the following function in the controller together with this fix

clever approach, but I have a couple of thoughts on this:

Firstly it makes sense to keep the controller logic as much agnostic to HTMX as possible IMO – even if it's unlikely that the same controller is used for regular HTML and HTMX in Neos Backend modules, it might be more common in other scenarios (and I think it's a good heuristic to keep the controller as slim as possible in any case).

Also I think it would make sense to come up with a general way of handling exceptional cases with HTMX. E.g. we could use a custom error handler that renders exceptions as Problem Details and then have some HTMX magic to turn that into an error flashmessage if needed.

Lastly, the default flash message container implementation starts a session every time a flash message is used. Again, in Neos Backend that's not an issue because you usually already have a session. But I think it would be nice to come up with a general concept that works in other contexts, too. E.g. sending flashmessages via custom HTTP header (as once mentioned in this ancient post)

Having written all this (sorry for getting side-tracked): The above sounds more complex than it hopefully is. I'll experiment with that and will let you know – I don't think that it will be a major effort to change the affected code afterwards

@Sebobo Sebobo requested a review from kdambekalns July 10, 2024 08:15
@bwaidelich
Copy link
Member

@Sebobo I added an example for flash messages (and CSRF token) to my HTMX example package: bwaidelich/Wwwision.HtmxTest@d7a3bda

@kitsunet kitsunet self-requested a review July 11, 2024 07:46
Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, not much conflict for modules so I do not envision that many problems. I guess preferably this would not go in an released version as it could in theory mess with existing modules if you did some weird stuff. But that seems like a pretty big edge case.

@Sebobo
Copy link
Member Author

Sebobo commented Jul 11, 2024

Makes sense to me, not much conflict for modules so I do not envision that many problems. I guess preferably this would not go in an released version as it could in theory mess with existing modules if you did some weird stuff. But that seems like a pretty big edge case.

I thought about that and the effect could be that someone wanted a status code or header, it never worked and they forgot to remove the piece of code and with this fix it would suddenly "do something".
Could be that even one of my packages does it, though I couldn't find any module in my projects that use throwStatus or setHeader in a ModuleController.

@bwaidelich bwaidelich merged commit 8162fb3 into 8.3 Jul 12, 2024
14 checks passed
@bwaidelich bwaidelich deleted the bugfix/set-response-code branch July 12, 2024 11:38
mhsdesign added a commit to kitsunet/neos-development-collection that referenced this pull request Sep 12, 2024
neos-bot pushed a commit to neos/neos that referenced this pull request Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants