Adding <meta> descriptions and Laravel's slug generator function with transliteration #1385
Conversation
If there is no slug for the discussion then the '-' separator is redundant and should not be used.
luceos
left a comment
luceos
left a comment
There was a problem hiding this comment.
Please drop the deprecated Str class completely. We're in beta, we're allowed to make backward breaking changes 👍
Aside from this are you 100% sure you replaced all instances of our old Str class in the source?
Required follow up is refactoring extensions to stop using our Str class.
|
@luceos That's fine. I made dropped the class and with this the whole Flarum\Util namespace is also gone. I confirm that before this PR I did a search and I cannot find any remaining usages of Flarum\Util\Str within the core or the bundled extensions. There were many usages of the Illuminate/Support/Str though. |
|
Note: This may close #194 |
src/Discussion/Discussion.php
Outdated
| $this->attributes['title'] = $title; | ||
| $this->slug = Str::slug($title); | ||
| $app = Application::getInstance(); | ||
| $locale = $app->make(LocaleManager::class)->getLocale() ?? 'en'; |
There was a problem hiding this comment.
This locale depends on the currently active user. Shouldn't we always use either en or at least the board's default language here, so that we get consistent results?
There was a problem hiding this comment.
Good point, I think the default locale would make the most sense.
johannsa
changed the title
|
@franzliedke, @luceos The changes requested were made. Also, I am including the changes I was making to add description. |
| */ | ||
| protected function getDescriptionAttribute($description) | ||
| { | ||
| $description = ($description == null) ? $this->startPost->content : $description; |
There was a problem hiding this comment.
This is gonna be inefficient when listing discussions D:
There was a problem hiding this comment.
@tobscure Are you referring to $this->startPost->content, right? The other option is to always have a description (from discussion creation) on the database, not only when you (i.e.: via 3rd party extension) want to have a custom non-generated description there (that was my original intention when adding the field).
I guess we could:
- Store a generated (first 300 characters plus '...' if needed) description upon discussion creation and/or update.
- In case the forum admin is interested in custom descriptions a third party extension would have to set the description field and prevent it from being edited (via additional listeners and maybe additional field) in case the OP edits the start post.
What do you think about this approach?
There was a problem hiding this comment.
Yeah I think we need to do something like this - any better ideas @franzliedke?
There was a problem hiding this comment.
The line we're talking about could also have been improved with the null coalescence operator.
I, however, agree to store a generated description, but only if none is set beforehand.
|
Tasks
|
|
@johannsa Are you still interested in finishing this? We'd highly appreciate it. 😃 |
|
@johannsa If you're interested in finishing this, it would be amazing if you could extract the slug generation changes into a separate PR so we could get that part merged in soon! Otherwise I will close this. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. |
|
I extracted the switch to Laravel's slugger (that has basic transliteration support) into a new PR - #1975. |
4 participants
Without additional dependencies we can fix the issue of some non-ASCII characters disappearing from the slugs (replaced by separator) by using the more complete slug() function on Illuminate\Support\Str. The entire namespace Flarum\Util, containing only Flarum\Util\Str, would be redundant and is marked as deprecate.
Update: I have repurposed this PR to include my work on adding descriptions for the core and, on separate PR, for the flarum-ext-tags extension.
What do you think about this?