Skip to content

Document the seven extenders that had no documentation at all - #597

Open
karl-bullock wants to merge 2 commits into
flarum:mainfrom
karl-bullock:docs-undocumented-extenders
Open

karl-bullock wants to merge 2 commits into
flarum:mainfrom
karl-bullock:docs-undocumented-extenders

Conversation

@karl-bullock

Copy link
Copy Markdown
Member

Nine pieces of extender API were documented nowhere at all: the Auth, Csrf, ErrorHandling, Link, ModelPrivate, Session and Validator extenders, plus Settings::registerLessConfigVar and Settings::resetJsCacheFor. This covers all of them except registerLessConfigVar, which #594 documents alongside the Theme extender.

Each one is put where a reader would go looking for it, rather than gathered into a reference dump.

Three new pages

extend/validation.md covers using a validator, writing one on AbstractValidator, and changing someone else's with the Validator extender. It opens by separating this from field rules on API resources, because in 2.0 that is where API input is actually validated and the two are not interchangeable: field rules for anything arriving over the API, AbstractValidator for data you are about to save from a command, a job or a listener. It documents the 2.0 behaviour that only rules whose key is present in the passed array are applied, and validateMissingKeys() for when you want the opposite. It also records that DiscussionValidator, PostValidator, TagValidator, SuspendValidator and GroupValidator were removed in 2.0, and that the Validating event is gone, since an extension being ported will look for both.

extend/error-handling.md explains the pipeline: an exception gets an error type and an HTTP status, the type is what selects a translated message or view, and anything unrecognised becomes unknown/500 and goes to the reporters. Then KnownError for your own exceptions, type() for third-party ones, status(), custom handlers returning a HandledError with details, and reporters. Core's registered statuses are included as a table so an extension can reuse an existing type rather than invent a parallel one.

extend/authentication.md covers password checkers and session drivers, and cross-references Authorization for the "what may they do" half. The checker return contract gets particular attention, because null and false are not interchangeable: null means "no opinion, let other checkers run" and false halts evaluation outright.

Four additions to existing pages

  • formatting.md gains the Link extender. It belongs there rather than on a page of its own: Link is implemented on top of the Formatter extender that page already documents, rewriting rel and target on URL tags at render time.
  • middleware.md gains CSRF route exemption, noting that exemptRoute() takes the route's name and not its path.
  • settings.md gains resetJsCacheFor.
  • model-visibility.md gains a "Private Models" section for ModelPrivate. That is the right page for it: the extender's own docblock describes the pairing with a viewPrivate scoper, and flarum/approval is built on exactly that combination, registering ModelPrivate checkers for Discussion and CommentPost and a viewPrivate scoper to reveal them.

sidebars.js registers the three new pages: authentication beside authorization and validation beside forms under Reference Guides, error handling beside database under Advanced Guides.

Where I added warnings

Four of these extenders fail in ways worse than a display bug, so each carries a short warning rather than being documented neutrally:

  • Removing the standard password checker makes the stored hash authenticate nobody, which can lock out every account including the admin account you would use to undo it.
  • A checker returning true is a full authentication bypass if it is wrong, so the page says to use hash_equals(), not to log the plaintext, and to keep custom checkers narrow.
  • A CSRF-exempt route has to establish its caller some other way, since any page anywhere can now invoke it with the visitor's cookies.
  • A privacy checker is re-evaluated on every save and sets is_private both ways, so one that stops matching will quietly republish a model.

Verification

npx docusaurus build --locale en completes with no errors and all three pages render. Docusaurus's broken-anchor check reports nothing for any page in this PR (model-visibility.md has one pre-existing broken anchor that #592 fixes, unrelated to the section added here). The sidebar resolves with no dangling entries.

Every signature, default value and return contract was read from the class in flarum/framework@2.x, including checking that serializeToForum takes the attribute name before the setting key, that Routes::post takes path then name then handler, and that ModelVisibility::scope takes the ability as its second argument.

One note on prior art

While checking for overlap I found as/validation, a branch from October 2020 with a complete extend/validation.md draft that was never merged. It is 326 commits behind and its examples use GroupValidator and the Validating event, both since removed, so I have not built on it, but the page it intended clearly wanted to exist and the structure here follows the same shape. Worth closing that branch if nobody is holding it. Same for as/post-types from August 2020, relative to #594.

Auth, Csrf, ErrorHandling, Link, ModelPrivate, Session and Validator were not mentioned anywhere in the docs, nor were Settings::resetJsCacheFor and the Theme extender (Theme is covered separately). Each is given a home where a reader would look for it rather than collected into a reference dump.

Three new pages. validation.md covers using and writing an AbstractValidator and extending someone else's with the Validator extender, and opens by distinguishing it from the field rules on API resources, which is where API input is actually validated in 2.0. It also records that DiscussionValidator, PostValidator, TagValidator, SuspendValidator and GroupValidator were removed in 2.0, along with the Validating event that used to be the way to adjust rules. error-handling.md explains the type and status pipeline, KnownError, custom handlers returning a HandledError, and reporters, with core's registered statuses as a table so an extension can reuse a type instead of inventing one. authentication.md covers password checkers, including why a checker should return null rather than false, and session drivers.

Four additions to existing pages. formatting.md gains the Link extender, since it is the same TextFormatter layer the page already documents. middleware.md gains CSRF route exemption. settings.md gains resetJsCacheFor. model-visibility.md gains private models, which is where ModelPrivate belongs: its own docblock describes the pairing with a viewPrivate scoper, and flarum/approval is built on exactly that.

Every signature, default and return contract comes from the class in flarum/framework@2.x. Security-sensitive extenders carry a warning where the failure mode is worse than a bug: removing the standard password checker can lock out every account, a CSRF-exempt route has to authenticate its caller some other way, and a privacy checker that stops matching will republish a model on its next save.

:::

## Session Drivers

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd be careful here... I understand where you're coming from, but we don't want to give the impression that setting the redis session driver by hand is a good thing.. fof/redis wraps all this so that the default Laravel implementation becomes compatible with Flarum's "quirks", for example

Comment thread docs/extend/error-handling.md Outdated

return [
(new Extend\ErrorHandling())
->reporter(SentryReporter::class),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Confused here, this would break the sentry extension, if overriding the reporter like this?

Comment thread docs/extend/formatting.md
use Psr\Http\Message\UriInterface;

return [
(new Extend\Link())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be best to call this out as an example, and note that fof/seo handles cases like this for the vast majority?

Comment thread docs/extend/settings.md Outdated
])
```

### Recompiling Assets When a Setting Changes

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not needed after rc.8, 2.0.0 covers this nicely (see 2.x-dev)

…tJsCacheFor

- Session drivers: do not suggest setting a redis driver by hand; Laravel's
  built-in drivers resolve but Flarum does not wire their cache stores.
  fof/redis is the supported route. Example driver renamed to acme.
- Reporters: example no longer named SentryReporter; note fof/sentry exists
  and that a second Sentry reporter double-reports.
- Link: frame as an example, point to fof/seo, note rel precedence, and
  correct when the URI is null.
- Settings: remove the resetJsCacheFor section; its example paired it with a
  runtime-serialized setting, and 2.x flags assets dirty itself.
@karl-bullock

Copy link
Copy Markdown
Member Author

Thanks Ian, all four were fair. I made the doc changes and used AI to check every reference against my edits, and it got these wrong. I'm on it.

@karl-bullock

Copy link
Copy Markdown
Member Author

Pushed:

  • Sessions: removed the hand-set redis example, now points to fof/redis.
  • Reporter: renamed the example from SentryReporter, with a pointer to fof/sentry.
  • Link: framed as an example, with fof/seo recommended first.
  • Settings: removed the resetJsCacheFor section.

This branch has not been deployed

No deployments
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.

2 participants