-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(Comments): sort by id (not type+id) in getMentions #56162
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -182,25 +182,14 @@ public function setMessage($message, $maxLength = self::MAX_MESSAGE_LENGTH): ICo | |||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * returns an array containing mentions that are included in the comment | ||||||||||
| * | ||||||||||
| * @return array each mention provides a 'type' and an 'id', see example below | ||||||||||
| * @psalm-return list<array{type: 'guest'|'email'|'federated_group'|'group'|'federated_team'|'team'|'federated_user'|'user', id: non-empty-lowercase-string}> | ||||||||||
| * @since 30.0.2 Type 'email' is supported | ||||||||||
| * @since 29.0.0 Types 'federated_group', 'federated_team', 'team' and 'federated_user' are supported | ||||||||||
| * @since 23.0.0 Type 'group' is supported | ||||||||||
| * @since 17.0.0 Type 'guest' is supported | ||||||||||
| * @since 11.0.0 | ||||||||||
| * @inheritDoc | ||||||||||
| */ | ||||||||||
| public function getMentions(): array { | ||||||||||
| $ok = preg_match_all("/\B(?<![^a-z0-9_\-@\.\'\s])@(\"(guest|email)\/[a-f0-9]+\"|\"(?:federated_)?(?:group|team|user){1}\/[a-z0-9_\-@\.\' \/:]+\"|\"[a-z0-9_\-@\.\' ]+\"|[a-z0-9_\-@\.\']+)/i", $this->getMessage(), $mentions); | ||||||||||
| if (!$ok || !isset($mentions[0])) { | ||||||||||
| return []; | ||||||||||
| } | ||||||||||
| $mentionIds = array_unique($mentions[0]); | ||||||||||
| usort($mentionIds, static function ($mentionId1, $mentionId2) { | ||||||||||
| return mb_strlen($mentionId2) <=> mb_strlen($mentionId1); | ||||||||||
| }); | ||||||||||
| $result = []; | ||||||||||
| foreach ($mentionIds as $mentionId) { | ||||||||||
| // Cut-off the @ and remove wrapping double-quotes | ||||||||||
|
|
@@ -237,6 +226,10 @@ public function getMentions(): array { | |||||||||
| $result[] = ['type' => 'user', 'id' => $cleanId]; | ||||||||||
| } | ||||||||||
| } | ||||||||||
| // Return sorted by id (descending length) | ||||||||||
| usort($result, static function ($mention1, $mention2) { | ||||||||||
| return mb_strlen($mention2['id']) <=> mb_strlen($mention1['id']); | ||||||||||
| }); | ||||||||||
|
Comment on lines
+230
to
+232
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
While at it ;) |
||||||||||
| return $result; | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -121,13 +121,19 @@ public function getMessage(); | |||||||
| public function setMessage($message, $maxLength = self::MAX_MESSAGE_LENGTH); | ||||||||
|
|
||||||||
| /** | ||||||||
| * returns an array containing mentions that are included in the comment | ||||||||
| * Returns all mentions from the comment message. | ||||||||
| * | ||||||||
| * @return array each mention provides a 'type' and an 'id', see example below | ||||||||
| * Parses the message, returning an array of mentions. | ||||||||
| * Mentions are sorted by descending length of their id before being returned. | ||||||||
| * | ||||||||
| * Supported mention types: 'user', 'group', 'team', 'guest', 'email', 'federated_group', 'federated_team', 'federated_user'. | ||||||||
| * | ||||||||
| * @return array Each mention is an associative array with 'type' and 'id' keys, sorted by descending length. | ||||||||
| * @psalm-return list<array{type: 'guest'|'email'|'federated_group'|'group'|'federated_team'|'team'|'federated_user'|'user', id: non-empty-lowercase-string}> | ||||||||
|
Comment on lines
+131
to
132
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Merge the two @return annotation |
||||||||
| * @since 30.0.2 Type 'email' is supported | ||||||||
| * @since 29.0.0 Types 'federated_group', 'federated_team', 'team' and 'federated_user' are supported | ||||||||
| * @since 23.0.0 Type 'group' is supported | ||||||||
| * @since 21.0.1 Sort returned results by | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. by what? |
||||||||
| * @since 17.0.0 Type 'guest' is supported | ||||||||
| * @since 11.0.0 | ||||||||
| */ | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would completely remove that block and add a
#[Override]instead