Slug Driver Support #2456

tankerkiller125 · 2020-11-17T03:48:04Z

Partially Solves flarum/issue-archive#203

Changes proposed in this pull request:

Implements drivers for slugging for discussions and users, with the option to update slugs for other models as well.

Reviewers should focus on:

Are we happy with this?

Confirmed

Frontend changes: tested on a local Flarum installation.
Backend changes: tests are green (run composer test).

Required changes:

Related core extension PRs: Add slug driver translations lang-english#179

askvortsov1

TODO:

Add an extender for adding slugging drivers to models. Add integration tests for that extender
Can we encapsulate any of the direct querying done in the drivers into the User / Discussion Repository classes, and inject those via DI?
@flarum/core Confirm our stance on calling the full slug (whatever it might be) slug, and serializing it as such to the frontend. This conflicts with the current naming used by the tags extension (so that's a breaking change if we adopt it there), but it would be nice to use a common convention, and just slug seems sensible (I would also be fine with fullSlug, but Clark I know you have concerns about that, and I see where you're coming from).
Decide whether we want to keep the toResource URL generator; if so, adopt throughout the core codebase where possible, and add an extender / tests for it
Figure out what to do with frontend code that obtains discussion ID from the slug. This happens in 2 places:
- DiscussionListItem's isActive, which can be overriden, but could also use app.current.discussion (which might make more sense).
- DiscussionPageResolver's getDiscussionIdFromSlug helper. I don't think there's a way to universalize this one, but IMO we should move it onto the DiscussionPageResolver class so it can be overriden to generate the key in some different manner if needed by an extension.

tankerkiller125 · 2020-11-24T00:13:26Z

Regarding @askvortsov1 points above

Will be done shortly
We did not make any changes here, and likely won't
Keeping it slug as fullSlug no longer makes sense given that drivers will allow devs to completely replace the slugging logic
Keeping toResource()

tankerkiller125 · 2020-11-25T17:01:42Z

@askvortsov1 I'm going to rebase this tonight when I get home so we can fix whatever Admin stuff we need to (Admin UX got merged) and from there this PR is good to go if I'm not mistaken?

askvortsov1 · 2020-11-25T19:12:33Z

Yep! It should be good for full review then.

tankerkiller125 · 2020-11-26T13:22:16Z

Rebased this onto the master with the Admin UX changes, everything looks great and works correctly in my testing. We still need to create the translations however.

js/src/common/components/Select.js

src/Discussion/IdWithSlugDriver.php

src/Admin/Content/AdminPayload.php

src/Http/HttpServiceProvider.php

src/Http/UrlGenerator.php

src/User/UsernameSlugDriver.php

[ci skip] [skip ci]

luceos

This is looking amazing, I am literally stunned by the implementation. Well done!

I left some considerations, questions; please have a look.

src/Api/Controller/ShowDiscussionController.php

luceos · 2020-12-02T09:09:45Z

src/Api/Serializer/BasicDiscussionSerializer.php

@@ -35,7 +46,7 @@ protected function getDefaultAttributes($discussion)

        return [
            'title' => $discussion->title,
-            'slug'  => $discussion->slug,
+            'slug' =>  $this->slugManager->forResource(Discussion::class)->toSlug($discussion),


Wouldn't it be easier to have a method on the abstract model instead?

$discussion->slugger->slug();

So that we resolve the slug manager late here as well, plus we can reduce the overhead on every, single class that tries to resolve the slug manager..

I'm a bit confused how that would work tbh. I'm very wary about adding stuff to AbstractModel unless it absolutely affects ALL models, which isn't the case here.

What if it's a trait one can apply to the model, eg Discussion has the Sluggable trait. It adds that trait, sets the property protected $slugger = DiscussionSlugger::class; and the trait handles the rest?

I don't like this. It adds unnecessary static stuff to the models, and I'd really prefer to move away from that as much as we possibly can because it's unnecessary leakage of global state. It also makes the extension mechanism a lot more hacky (similar to what I had to do with #2460), which I really didn't like and will revisit again after stable.

src/Forum/Content/Discussion.php

SychO9

Looking good! nice work!

I believe it'd be better to have IdOnlyUserSlugDriver here instead of the nicknames extension.

If I make for example another extension that adds a different user display name driver, the admin might want to use this IdOnlyUserSlugDriver but then I'd have to also add it to my own extension, because it only resides within the nickname extension.

Let me know what you think.

src/Discussion/IdWithTransliteratedSlugDriver.php

SychO9 · 2020-12-03T17:14:04Z

js/src/admin/components/BasicsPage.js

+      this.slugDriverOptions[model] = {};
+
+      app.data.slugDrivers[model].forEach((option) => {
+        this.slugDriverOptions[model][option] = option;


Are the keys getting localized somewhere ? I mean default and idOnly for example (same with display name drivers) They don't seem to be, at least the option's label not the key itself.

Nope, and mail drivers aren't either. That's a limitation worth revisiting later IMO

I see now, something like app.translator.trans('core.admin.basics.slug_drivers.${option}') wouldn't work, since extensions are adding drivers, and that means they couldn't add locales for the drivers anyway.

The solution would need to be more sophisticated. Yeah worth revisiting it later.

src/Http/HttpServiceProvider.php

askvortsov1

I believe it'd be better to have IdOnlyUserSlugDriver here instead of the nicknames extension.

On second thought, I agree with this, and that goes for IdOnlyDiscussionSlugDriver as well (and that doesn't break anything with the frontend stuff since the ID is still first).

SychO9

I have nothing else to add, it all looks good to me.

Just noting that we need of course to remember to add the id only drivers in core and to add the nicknames extension to composer.json

askvortsov1 mentioned this pull request Nov 17, 2020

Cleaned up discussion idWithSlug handling #2046

Closed

2 tasks

askvortsov1 requested changes Nov 19, 2020

View reviewed changes

tankerkiller125 force-pushed the mk/2230-utf-8 branch from f2c7030 to 1eb316b Compare November 26, 2020 13:21

tankerkiller125 requested review from askvortsov1, clarkwinkelmann, luceos and SychO9 November 26, 2020 13:22

tankerkiller125 marked this pull request as ready for review November 26, 2020 13:22

askvortsov1 requested changes Nov 26, 2020

View reviewed changes

askvortsov1 approved these changes Nov 30, 2020

View reviewed changes

tankerkiller125 and others added 16 commits November 30, 2020 19:25

Initial Driver Support (No Extender)

44902e9

Apply fixes from StyleCI

668962d

[ci skip] [skip ci]

Fix user ShowTests

0358161

Format

0fc6dfc

WIP Extender

79721bc

Apply fixes from StyleCI

1d260b9

[ci skip] [skip ci]

Slug Extender Test works

45db62e

Apply fixes from StyleCI

4791741

[ci skip] [skip ci]

Working Admin Page

421aabd

Made frontend discussion slug independent

adc2fce

Apply fixes from StyleCI

ca1efe0

[ci skip] [skip ci]

Make min search length a constant

be42a0b

Fix some review issues

2c0afc2

Use Repositories instead of models

7b9e829

Apply fixes from StyleCI

a9ab7e4

[ci skip] [skip ci]

Remote UrlGenerator toResource

c90ea80

luceos and others added 4 commits November 30, 2020 19:25

Apply fixes from StyleCI

5ea83f7

[ci skip] [skip ci]

Fixing UserRepository

60edecc

Trying process isolation to fix weird test bug

6e255f8

Try to fix test

2ac869f

tankerkiller125 force-pushed the mk/2230-utf-8 branch from d5e6c2d to 2ac869f Compare December 1, 2020 00:41

luceos requested changes Dec 2, 2020

View reviewed changes

askvortsov1 requested a review from luceos December 2, 2020 19:40

Fix parameter order

1db1d11

SychO9 reviewed Dec 3, 2020

View reviewed changes

askvortsov1 requested changes Dec 3, 2020

View reviewed changes

SychO9 approved these changes Dec 5, 2020

View reviewed changes

Clean up ternary

572e5a4

askvortsov1 approved these changes Dec 5, 2020

View reviewed changes

luceos approved these changes Dec 7, 2020

View reviewed changes

askvortsov1 merged commit 4679448 into master Dec 7, 2020

askvortsov1 deleted the mk/2230-utf-8 branch December 7, 2020 18:33

askvortsov1 mentioned this pull request Dec 8, 2020

Usernames cannot be numeric #1356

Closed

clarkwinkelmann mentioned this pull request Aug 11, 2021

Tag nojs page doesn't use discussion slugger #3013

Closed

PipecraftNet mentioned this pull request Sep 7, 2021

v1.1.0 Register a discussion ID slug driver PipecraftNet/flarum-ext-id-slug#2

Merged

askvortsov1 mentioned this pull request Nov 26, 2020

When searching for something with few characters doesn't throw error. flarum/issue-archive#368

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slug Driver Support #2456

Slug Driver Support #2456

tankerkiller125 commented Nov 17, 2020 •

edited

Loading

askvortsov1 left a comment

tankerkiller125 commented Nov 24, 2020 •

edited

Loading

tankerkiller125 commented Nov 25, 2020

askvortsov1 commented Nov 25, 2020

tankerkiller125 commented Nov 26, 2020

luceos left a comment

luceos Dec 2, 2020

askvortsov1 Dec 2, 2020

luceos Dec 3, 2020

askvortsov1 Dec 3, 2020

SychO9 left a comment •

edited

Loading

SychO9 Dec 3, 2020

askvortsov1 Dec 3, 2020

SychO9 Dec 3, 2020

askvortsov1 left a comment

SychO9 left a comment

Slug Driver Support #2456

Slug Driver Support #2456

Conversation

tankerkiller125 commented Nov 17, 2020 • edited Loading

askvortsov1 left a comment

Choose a reason for hiding this comment

tankerkiller125 commented Nov 24, 2020 • edited Loading

tankerkiller125 commented Nov 25, 2020

askvortsov1 commented Nov 25, 2020

tankerkiller125 commented Nov 26, 2020

luceos left a comment

Choose a reason for hiding this comment

luceos Dec 2, 2020

Choose a reason for hiding this comment

askvortsov1 Dec 2, 2020

Choose a reason for hiding this comment

luceos Dec 3, 2020

Choose a reason for hiding this comment

askvortsov1 Dec 3, 2020

Choose a reason for hiding this comment

SychO9 left a comment • edited Loading

Choose a reason for hiding this comment

SychO9 Dec 3, 2020

Choose a reason for hiding this comment

askvortsov1 Dec 3, 2020

Choose a reason for hiding this comment

SychO9 Dec 3, 2020

Choose a reason for hiding this comment

askvortsov1 left a comment

Choose a reason for hiding this comment

SychO9 left a comment

Choose a reason for hiding this comment

tankerkiller125 commented Nov 17, 2020 •

edited

Loading

tankerkiller125 commented Nov 24, 2020 •

edited

Loading

SychO9 left a comment •

edited

Loading