Tags GlobalPolicy using tags.position!=null to identify primary tags??

echolocked

Hi. First I'm new to the community so I'm not sure if Dev is the right tag to post. Please feel free to retag if I'm posting in the wrong place.

TLDR: what's the right way to arrange primary tags and secondary tags hierarchically? (for example, putting Extensibility, Internal, i18n, Theming under Dev)?

Details:

I arranged test tags structure like this:

I structured the tags this way because I figured I can nicely layout the tags in a nested display. But this way changes the position of secondary tags, which caused unexpected behavior:

The GlobalPolicy::can() method in the tags extension denies startDiscussion permission, when users have the global startDiscussion permission and all tags are non-restricted, because it's using tags.position=null to count primary/seconday tags.

why not use is_primary?

$primaryTagsWhereHasPermission = Tag::whereHasPermission($actor, $ability) ->where('tags.position', '!=', null) ->count();

$secondaryTagsWhereHasPermission = Tag::whereHasPermission($actor, $ability) ->where('tags.position', '=', null) ->count();

So, 1. how do I nest secondary tags under primary tags? (tried dragging in Admin but it aligns secondary with primary tags, but I would be doing it wrong)

what's the right way of identify primary tag? position=null or is_primary=1?

luceos

echolocked terminology here is key, let me explain the options:

Primary tags, tags sorted in the Primary Tags that are not a child of another primary tag.
Child or nested tag, tags sorted in the Primary Tags that are a child of a primary tag.
Helper tags, tags listed in secondary tags.

(ps this is my own preference in terminology because the vanilla Flarum one is highly confusing)

Now based on your questions I assume the answers:

Tags can be dragged from one column to the other; you can make a helper tag a primary or nested one. Just tested it here on discuss.
is_primary distinguishes secondary (helper) with primary (primary or child), position = null is a primary tag that is not a child.

Hope this helps, let me know if you have any further questions.

echolocked

luceos Thanks for your explanation. Some followup quesitons:

the terminology is fine, but seems there's inconsistency across different parts of Flarum's tags system:

Tag.ts:
isPrimary() { return position !== null && parent === false }
Permission System (GlobalPolicy.php):
// Line 39 - Primary tags $primaryTagsWhereHasPermission = Tag::whereHasPermission($actor, $ability) ->where('tags.position', '!=', null) ->count();

// Line 53 - Secondary tags $secondaryTagsWhereHasPermission = Tag::whereHasPermission($actor, $ability) ->where('tags.position', '=', null) ->count();
- Primary: position != null (regardless of parent)
- Secondary: position = null (regardless of parent)
Validation System, same as Tag.ts
- v1.8.x (SaveTagsToDatabase.php):
- v2.x (DiscussionResourceFields.php):
  if ($tag->position !== null && $tag->parent_id === null) { $primaryCount++; } else { $secondaryCount++; }

I observed unexpected behavior which I described earlier that can() permission was denied when users should be allowed to create discussions

Just trying to understand if this is intentional.

Context is I'm seeking the possibility of adding a user defined tag extension which can bypass the performance issue caused by permission checking.

luceos

echolocked I tried answering a question from memory without truly understanding whether that was alright. Here it goes based on the db scheme using Flarum terminology 😉

Primary tags from the left column have a boolean (int 1) field is_primary. This distinguishes them from secondary tags. Nested tags of primary tags can be distinguished using their parent_id field, null in case of a root primary tag, otherwise it contains the id of the parent primary tag. Primary tags also have position set to sort them in the frontend. This applies to their nested tags as well, though their position starts anew. Secondary (helper tags) don't have a position, they're sorted based on use (I believe).

As for the inconsistencies, it is possible that a refactoring is required. The fact we have an is_primary field but the code still deduces it from position and parent is weird. Flarum is open source and spotting these kind of possible improvements is not happening that often. Would you be interested in helping us restructure this extension for 2.x perhaps?