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

ENH additional error codes, tidy translations #89

Merged

Conversation

andrewandante
Copy link
Contributor

Fixes #78

Adds in 418 and a few others (source: https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#4xx_client_errors and https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#5xx_server_errors)

Cleaned up a couple of en translations that still referenced Base Pages

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Seems sensible, thank you!
I'm not a fan of the "I'm a teapot" error being treated as a "real" error myself, and agree with Max on the original issue that this is likely to confuse content editors... but ultimately it sounds like the consensus is that since it's in the spec it should be included. That's hard to argue against, so I'll accept that it's there.

Will merge on green.

@andrewandante
Copy link
Contributor Author

Thanks :) My thought while doing this was to make the list of codes configurable somehow, but it started to feel unwieldy pretty quickly so I settled on this one.

Perhaps I'll create a separate issue that makes the list of codes to provide to the drop down configurable in yaml? Then limits it to something like 400, 401, 403, 404, 500, 503, 504 as a baseline?

@GuySartorelli
Copy link
Member

GuySartorelli commented Sep 25, 2023

Perhaps I'll create a separate issue that makes the list of codes to provide to the drop down configurable in yaml? Then limits it to something like 400, 401, 403, 404, 500, 503, 504 as a baseline?

Sounds like a good issue. I probably wouldn't limit it by default (some people might consider that a breaking change... it's a grey area as to whether it would be or not) - rather I'd include the full list in the default set, and developers can choose to hide any they want to by setting them to null

e.g. if it was defined as

private static array $error_codes = [
  '404' => 'not found'
  // etc
];

Then developers could add this to their yaml

SilverStripe\ErrorPage\ErrorPage:
  error_codes:
    418: null

Or perhaps have a separate disallowed_error_codes config? Or a allowed_error_codes config that is null by default but if it's an array, it overrides the default full set?

Anyway, all that to say "yes, please create an issue about it" - we can discuss implementation in that issue.

@GuySartorelli GuySartorelli merged commit a3f84c0 into silverstripe:2 Sep 25, 2023
@andrewandante andrewandante deleted the ENH/add_missing_error_codes branch September 25, 2023 02:13
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.

Error 418 missing
2 participants