-
-
Notifications
You must be signed in to change notification settings - Fork 836
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
Slug Driver Support #2456
Slug Driver Support #2456
Changes from 20 commits
44902e9
668962d
0358161
0fc6dfc
79721bc
1d260b9
45db62e
4791741
421aabd
adc2fce
ca1efe0
be42a0b
2c0afc2
7b9e829
a9ab7e4
c90ea80
5ea83f7
60edecc
6e255f8
2ac869f
1db1d11
572e5a4
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 |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
namespace Flarum\Api\Serializer; | ||
|
||
use Flarum\Discussion\Discussion; | ||
use Flarum\Http\SlugManager; | ||
use InvalidArgumentException; | ||
|
||
class BasicDiscussionSerializer extends AbstractSerializer | ||
|
@@ -19,6 +20,16 @@ class BasicDiscussionSerializer extends AbstractSerializer | |
*/ | ||
protected $type = 'discussions'; | ||
|
||
/** | ||
* @var SlugManager | ||
*/ | ||
protected $slugManager; | ||
|
||
public function __construct(SlugManager $slugManager) | ||
{ | ||
$this->slugManager = $slugManager; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
* | ||
|
@@ -35,7 +46,7 @@ protected function getDefaultAttributes($discussion) | |
|
||
return [ | ||
'title' => $discussion->title, | ||
'slug' => $discussion->slug, | ||
'slug' => $this->slugManager->forResource(Discussion::class)->toSlug($discussion), | ||
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. 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.. 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. 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. 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. 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 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. 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. |
||
]; | ||
} | ||
|
||
|
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.
Are the keys getting localized somewhere ? I mean
default
andidOnly
for example (same with display name drivers) They don't seem to be, at least the option's label not the key itself.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.
Nope, and mail drivers aren't either. That's a limitation worth revisiting later IMO
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 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.