Extract opengraph methods #10655

notbakaneko · 2023-10-19T09:21:30Z

directly continues Add more Opengraph headers to more pages #10639
moves the opengraph methods to their own helper classes instead of filling up the existing model files
no need to pass the variable into the view factory since only the metadata blade cares about it
wiki page parser now has a thing that pulls out first paragraph block as the excerpt (maybe news can use it too?)

Also I did think of removing the Opengraph suffixes from the class names, except, they're usually used in places where there'd already be an existing model class import with the same name, so they'd have to be aliased to as xxOpengraph anyway 👀

Not sure what to with the user multiplayer tabs descriptions as the room/playlist counts seems...not useful 🤔

…very line in

factory since only metadata view needs it.

nanaya · 2023-10-19T14:23:44Z

I'm skipping reviewing the other one

notbakaneko · 2023-10-20T09:23:52Z

Adding per beatmap opengraph on the beatmapset url isn't currently possible as the beatmap portion of the url is part of the fragment and client-side;
turns out the same is true for discussion posts, it's all client-side.

nanaya · 2023-10-19T13:10:45Z

app/helpers.php

@@ -1690,6 +1691,15 @@ function seeded_shuffle(array &$items, int $seed = 0)
    mt_srand();
 }

+function set_opengraph(HasOpengraph $model, ...$options)
+{
+    $className = 'App\\Libraries\\Opengraph\\'.get_class_basename(get_class($model)).'Opengraph';


should use plain replace otherwise models in subdirectory will be structured wrong (like the wiki page).

also, there's $model::class

nanaya · 2023-10-19T13:11:04Z

resources/views/layout/metadata.blade.php

@@ -6,6 +6,8 @@
    $appUrl = config('app.url');
    $currentLocale = App::getLocale();
    $fallbackLocale = config('app.fallback_locale');
+
+    $opengraph['description'] ??= $opengraph['description'] ?? $pageDescription ?? null;


redundant $opengraph['description']

nanaya · 2023-10-24T08:37:07Z

app/Models/Comment.php

+
+    private function getMessageHtml(): ?string
+    {
+        return $this->memoize(__FUNCTION__, function () {


use shorter fn?

nanaya · 2023-10-24T08:50:25Z

app/Libraries/Opengraph/PageOpengraph.php

+
+        return [
+            // TODO: need a way to mark which wiki text to use as excerpt; first_paragraph just returns the title on wiki.
+            'description' => html_excerpt($this->page->get()['output']),


error on nonexistent page

nanaya · 2023-10-24T12:16:48Z

app/helpers.php

+    View::share([
+        'opengraph' => (new $className($model, ...$options))->get(),
+    ]);


this needs to be reset after each request because the value persists to the next requests until overwritten

nanaya · 2023-10-26T12:05:47Z

resources/lang/en/users.php

@@ -124,6 +124,9 @@
    ],

    'ogp' => [
+        'modding_description' => ':user\'s beatmaps: :counts',
+        'modding_description_empty' => ':user doesn\'t have nay beatmaps...',


nanaya · 2023-10-27T09:03:57Z

app/Libraries/Opengraph/CommentOpengraph.php

+    {
+        $user = $this->comment->user;
+
+        return priv_check('CommentShow', $this->comment)->can()


priv_check_user with null user?

originally, I had everything assume opengrapgh would be viewed only by null user, but if you go to the page with a deleted comment as a moderator, you see a different thing, so I'm not so sure now 👀

nanaya · 2023-10-27T09:06:03Z

app/Libraries/Opengraph/CommentOpengraph.php

+
+        return priv_check('CommentShow', $this->comment)->can()
+            ? [
+                'description' => html_excerpt($this->comment->message_html, 100),


blade_safe? return of html_excerpt is already escaped

although I noticed discord double unescape description by default 🤔 and twitter is even worse

nanaya · 2023-10-27T09:10:15Z

app/Libraries/Opengraph/Wiki/PageOpengraph.php

+        $pageData = $this->page->get();
+
+        return [
+            'description' => $pageData['excerpt'] ?? html_excerpt($pageData['output']), // html_excerpt fallback


another one needing blade_safe

nanaya · 2023-10-27T09:42:53Z

app/Libraries/Markdown/Osu/DocumentProcessor.php

@@ -263,6 +269,15 @@ private function proxyImage()
        }
    }

+    private function recordExcerpt(): void
+    {
+        if ($this->excerpt !== null || !$this->node instanceof Paragraph || !$this->event->isEntering()) {


with first two paragraphs may not even contain actual content I wonder if it's better to just show first 100 characters instead.

This reverts commit 5927427.

The downside is it makes elasticsearch return more text that we don't need

site_name isn't displayed on most cards, also apparently should just be "osu!"?

nanaya · 2023-11-06T06:04:20Z

resources/views/layout/metadata.blade.php

+        @php
+            if ($key === 'title') {
+                $isHtmlString = $value instanceof HtmlString;
+                $value .= ' · '.page_title();
+                if ($isHtmlString) {
+                    $value = new HtmlString($value);
+                }
+            }
+        @endphp
+        <meta property="og:{{ $key }}" content="{{ $value }}">


a bit more repetitive but a lot shorter. (also the page_title() should be escaped)

Suggested change

@php

if ($key === 'title') {

$isHtmlString = $value instanceof HtmlString;

$value .= ' · '.page_title();

if ($isHtmlString) {

$value = new HtmlString($value);

}

}

@endphp

<meta property="og:{{ $key }}" content="{{ $value }}">

@if ($key === 'title')

<meta property="og:{{ $key }}" content="{{ $value }} · {{ page_title() }}">

@else

<meta property="og:{{ $key }}" content="{{ $value }}">

@endif

nanaya · 2023-11-06T12:30:16Z

resources/views/layout/metadata.blade.php

-@if (isset($opengraph))
-    <meta property="og:site_name" content="osu! » {{ page_title() }}">
-    <meta property="og:type" content="website">
+<meta property="og:site_name" content="osu! » {{ page_title() }}">


page title not needed anymore? Both og title and html title always have it now

nanaya · 2023-11-06T12:30:31Z

resources/views/layout/metadata.blade.php

@@ -3,9 +3,14 @@
    See the LICENCE file in the repository root for full licence text.
 --}}
 @php
+    use Illuminate\Support\HtmlString;


nanaya · 2023-11-06T12:34:51Z

app/Libraries/Opengraph/BeatmapsetOpengraph.php

+
+use App\Models\Beatmapset;
+
+class BeatmapsetOpengraph


implements Something? The set_opengraph assumes everything here have the get method.

nanaya · 2023-11-06T13:16:25Z

app/Libraries/Opengraph/HasOpengraph.php

+namespace App\Libraries\Opengraph;
+
+interface HasOpengraph
+{


I wonder if this should require something like $opengraphClass and use it for set_opengraph. Not sure if helps anything at all though. And this currently isn't enforcing anything. One can have HasOpengraph model without corresponding class and it'll only explode on runtime. That said, specifying the class name won't help either since it's not checked and can't specify inheritance either.

eh 🤷

I got rid of it since the type check is more of "belongs to App\Models" than anything 🤷

…be constant, anyway)

nanaya · 2023-11-07T05:49:28Z

app/Libraries/Opengraph/BeatmapsetOpengraph.php

+    public function get(): array
+    {
+        $section = osu_trans('layout.menu.beatmaps._');
+        $title = "{$this->beatmapset->artist} - {$this->beatmapset->title}"; // opengrah header always intended for guest.


oh right, opengraph missing p

doesn't actually implement anything, not really used for typecheck

notbakaneko added 11 commits October 16, 2023 15:04

add toOpengraph method

7df0115

more toOpengraph methods

6a3bf2b

move shared descriptions

3e84b70

add toOpengraph method for forum and topic show

13d0101

ogp description always same as meta description

d0c15f0

remove markdown/up from featured artist description; also don't put e…

12bb5cb

…very line in

removing toMetaDescription

e96033d

add missing opengraph for beatmapset

c24c508

add more cover images to opengraph headers

83d9ac4

add more titles

72fd995

move opengraph stuff to separate classes; skip passing variable to view

6c50026

factory since only metadata view needs it.

notbakaneko self-assigned this Oct 19, 2023

notbakaneko added 5 commits October 20, 2023 18:53

users.show.profile version of title not actually used on current site

92cb2fb

Merge branch 'master' into feature/opengraph-interface-2

1f6c85a

add opengraph for comment

23f63d0

memoize message_html

efe5cda

wiki page opengraph

7dbb641

notbakaneko added the type:behavioural label Oct 23, 2023

nanaya mentioned this pull request Oct 24, 2023

Add more Opengraph headers to more pages #10639

Closed

nanaya requested changes Oct 24, 2023

View reviewed changes

notbakaneko added 8 commits October 25, 2023 18:08

record excerpt from first text paragraph

5927427

no extra data for missing page

cfdf270

single line

32e692f

don't coalesce into self

991b6e2

non-model namespace can be problem for future (if at all)

c2b393f

set only on current request

bb95035

modding opengraph descriptions

911d0f4

single quotes

bde5e30

nanaya requested changes Oct 27, 2023

View reviewed changes

notbakaneko added 10 commits October 30, 2023 16:46

typo

0bf535b

already escaped

078e18f

Revert "record excerpt from first text paragraph"

6b5cef0

This reverts commit 5927427.

use the text-only searchable version for opengraph previews.

6188225

The downside is it makes elasticsearch return more text that we don't need

assume always guest for opengraph headers

ada79a7

opengraph title doesn't override titlePrepend

6560b14

remove username from description (standardized with profile info)

9a9fc1d

missing typehint

fb6f795

add page_title to opengraph title by default;

0bb17e0

site_name isn't displayed on most cards, also apparently should just be "osu!"?

rewrap in HtmlString after appending page_title

68451af

nanaya reviewed Nov 6, 2023

View reviewed changes

notbakaneko added 2 commits November 6, 2023 16:07

whoops

d2b1930

shorter

25afc43

nanaya requested changes Nov 6, 2023

View reviewed changes

notbakaneko added 2 commits November 7, 2023 14:35

unused

7a7967f

remove page_title from og:site_name (which apparently is supposed to …

6f52526

…be constant, anyway)

nanaya reviewed Nov 7, 2023

View reviewed changes

notbakaneko added 2 commits November 7, 2023 14:50

implemnent interface

5c466b7

remove kind of useless interface;

b2f6727

doesn't actually implement anything, not really used for typecheck

nanaya approved these changes Nov 7, 2023

View reviewed changes

Merge branch 'master' into feature/opengraph-interface-2

6911367

nanaya enabled auto-merge November 7, 2023 08:37

nanaya merged commit 5809146 into ppy:master Nov 7, 2023
3 checks passed

notbakaneko deleted the feature/opengraph-interface-2 branch November 13, 2023 07:05

cl8n mentioned this pull request May 23, 2024

Topic covers not viewable or editable #11226

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract opengraph methods #10655

Extract opengraph methods #10655

notbakaneko commented Oct 19, 2023 •

edited

Loading

nanaya commented Oct 19, 2023

notbakaneko commented Oct 20, 2023 •

edited

Loading

nanaya Oct 19, 2023

nanaya Oct 19, 2023

nanaya Oct 24, 2023

nanaya Oct 24, 2023

nanaya Oct 24, 2023

nanaya Oct 26, 2023

nanaya Oct 27, 2023

notbakaneko Oct 27, 2023

nanaya Oct 27, 2023

nanaya Oct 27, 2023

nanaya Oct 27, 2023

nanaya Nov 6, 2023

nanaya Nov 6, 2023

nanaya Nov 6, 2023

nanaya Nov 6, 2023

nanaya Nov 6, 2023

notbakaneko Nov 7, 2023

nanaya Nov 7, 2023

Extract opengraph methods #10655

Extract opengraph methods #10655

Conversation

notbakaneko commented Oct 19, 2023 • edited Loading

nanaya commented Oct 19, 2023

notbakaneko commented Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notbakaneko commented Oct 19, 2023 •

edited

Loading

notbakaneko commented Oct 20, 2023 •

edited

Loading