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

Remove some http_response_code()/exit()/die() usages #18068

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

cedric-anne
Copy link
Member

Checklist before requesting a review

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.

Description

Follows #18046 .

Usage of the http_response_code() method is not compatible with many Symfony features (as explained in #18014) and usage of the exit()/die() methods prevents some Symfony features to be used (e.g. kernel events listeners).

I replace some usages of these methods to make the corresponding code fully compatible with the Symfony framework.

@cedric-anne cedric-anne self-assigned this Oct 15, 2024
@cedric-anne cedric-anne force-pushed the 11.0/remove-exit-die-usages branch 2 times, most recently from 67b63ee to f9fb916 Compare October 15, 2024 14:05
@cedric-anne cedric-anne marked this pull request as ready for review October 16, 2024 06:13
@cedric-anne cedric-anne added this to the 11.0.0 milestone Oct 16, 2024
Copy link
Contributor

@trasher trasher left a comment

Choose a reason for hiding this comment

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

As discussed on our internal chat, there are some things to fix

'login_url' => $login_url,
]
)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Responses miss the HTTP code from the Exception objects

Copy link
Member Author

Choose a reason for hiding this comment

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

This page just displays a message indicating why authentication fails and a link to the login form. We could return a 400 error, but a 200 seems OK too. The authentication error could be related to many possible issues (unavailability of a remote system, bad credentials, ...) and would not always correspond to a bad request.

Anyway, it does not change much for the end user.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's perfectly fine to return a 4xx response that has HTML content with error messages, I'd even say it's recommended for clarity.
An HTTP 200 with an error message inside sounds like an "Error failed successfully" message 🤔

Strip-Response-code-650-finalenglish

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure 400 matches the different cases, but it is probably the more generic 4xx code that could be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants