silverstripe / silverstripe-errorpage

ErrorPage module for Silverstripe CMS
BSD 3-Clause "New" or "Revised" License
2 stars 17 forks source link

Make dropdown of error codes configurable in yaml #90

Closed andrewandante closed 1 year ago

andrewandante commented 1 year ago

Spawned as a result of #89

There are a lot of valid error codes, and for the sake of completionism it makes sense to have them all available from the dropdown. However, given these are available for content authors to use, it also makes sense to limit the default set.

It'd be great to move the list of codes to configuration, and set the default list to something like:

With the option to include all or customise the list that you'd like the authors to see.

PR

GuySartorelli commented 1 year ago

Moving my thoughts here:

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',
  '418' => 'teapot',
  // etc
];

Then developers could add this to their yaml

SilverStripe\ErrorPage\ErrorPage:
  error_codes:
    418: null

Alternatively, we could 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.

andrewandante commented 1 year ago

some people might consider that a breaking change

Ah that's a good point, I hadn't considered that.

I think I prefer the allowed_error_codes option, but I'm uncertain how that fits best with the translation strings. Maybe we leave the mapping as a static method somewhere, and have the int values of the codes be the configurable part?

i.e.

SilverStripe\ErrorPage\ErrorPage:
  allowed_error_codes:
    - 400
    - 401
    - 403
    - 404
    - 500

with essentially the same PHP that we have at the moment but also with:

if (Config::get(ErrorPage::class, 'allowed_error_codes') !== null) {
  .. only filter the codes for those ones ...
}
GuySartorelli commented 1 year ago

With the exception that I'd prefer static::config()->get('allowed_error_codes'), that sounds like a good solution to me.

GuySartorelli commented 1 year ago

Thanks for contributing that! What a great feature. It'll be included in Silverstripe CMS 5.2.0