Skip to content

Replace the tags policy examples with the 2.x code they claim to be - #602

Open
karl-bullock wants to merge 1 commit into
flarum:mainfrom
karl-bullock:docs-policy-examples
Open

karl-bullock wants to merge 1 commit into
flarum:mainfrom
karl-bullock:docs-policy-examples

Conversation

@karl-bullock

Copy link
Copy Markdown
Member

Both code samples under Example Policies are 1.x code presented as current Flarum Tags source, and the page links to that source, so a reader who follows the link finds something quite different from what they were just shown.

GlobalPolicy: the sample would not run

count(Tag::getIdsWhereCan($actor, $ability, true, false)) >= $this->settings->get('min_primary_tags')

Tag::getIdsWhereCan() does not exist anywhere in 2.x, and the setting keys lost their namespace at some point (flarum-tags.min_primary_tags now). So this is not merely dated: copying it produces a fatal.

It is replaced with a simplified policy assembled from real APIs, a ?string return, the namespaced setting, and the Tag::whereHasPermission scope. I have not pretended the real implementation is that short: a note says it is simplified, and that the real one also handles secondary tags, short-circuits on bypassTagCounts, memoizes its verdict per actor and ability, and counts both tag kinds in one aggregate query, which matters because a global policy runs on a great many requests.

TagPolicy: the sample misses the two things worth learning from it

The old sample defines one method per ability (startDiscussion, then addToDiscussion delegating to it) and wraps hasPermission in a ternary. The real one does neither:

  • It uses the can() catch-all, so a single method serves every ability. The page already explains that mechanism a few paragraphs earlier, and this is the example that should demonstrate it.
  • It returns the result of hasPermission directly, because AbstractPolicy::sanitizeResult converts true and false into allow() and deny(). The ternary is noise.
  • It also inherits restrictions from a parent tag, which is a real behaviour of the extension that the sample silently dropped.

The sample now matches the real class, and the prose points out those two habits rather than leaving the reader to notice them.

Note on overlap

The introductory line above the first sample is deliberately untouched, because #592 corrects the branch in its link (blob/master to blob/2.x). That matters here: once that link points at 2.x, the mismatch between the page and the linked source becomes obvious, which is how I found this. I verified this branch and #592 merge cleanly.

Verified with npx docusaurus build --locale en: no errors, and no anchors broken by this change (the two the build reports on these pages are pre-existing and fixed by #592).

Both examples in Example Policies are 1.x code presented as current Flarum Tags source, and the page links to that source, so a reader who follows the link finds something quite different.

The GlobalPolicy sample calls Tag::getIdsWhereCan(), which does not exist anywhere in 2.x, and reads un-namespaced settings (min_primary_tags rather than flarum-tags.min_primary_tags), so it is not merely dated but would not run. It is replaced by a simplified policy built from real APIs: a nullable-string return, the namespaced setting, and the Tag::whereHasPermission scope. A note says plainly that it is simplified and why the real one is more involved, rather than pretending the real implementation is this short.

The TagPolicy sample defined one method per ability and wrapped hasPermission in a ternary. The real one uses the can() catch-all so a single method serves every ability, inherits restrictions from a parent tag, and returns the boolean from hasPermission directly, since AbstractPolicy::sanitizeResult turns true and false into allow() and deny(). Those are the two habits worth copying from it, so the sample now shows them and the prose points them out.

The intro line is deliberately untouched, because flarum#592 corrects the branch in its link.

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.

1 participant