feat: add support for customizable user avatar drivers#4130
feat: add support for customizable user avatar drivers#4130datlechin wants to merge 16 commits intoflarum:2.xfrom
Conversation
|
Hey @datlechin sorry this has been dormant for so long. Do you have any interest in continuing user avatar drivers? If so, please would you rebase and resolve the conflicts here? I'd really like to include this before the 2.0 stable release if possible 🙏 |
There was a problem hiding this comment.
Pull request overview
This pull request introduces a customizable avatar driver system that allows extensions to provide alternative avatar sources (e.g., Gravatar) when users don't have uploaded avatars. The implementation mirrors the existing display name driver pattern.
Key Changes:
- Added avatar driver interface and default implementation that returns null for users without custom avatars
- Introduced
originalAvatarUrlattribute to distinguish between uploaded avatars and driver-generated ones - Extended admin UI to allow configuring avatar drivers when multiple options are available
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
framework/core/src/User/UserServiceProvider.php |
Registers avatar driver system with container bindings and default driver |
framework/core/src/User/User.php |
Adds avatar driver property, setter method, and modified getAvatarUrlAttribute to use driver as fallback |
framework/core/src/User/Avatar/DriverInterface.php |
New interface defining the contract for avatar drivers |
framework/core/src/User/Avatar/DefaultDriver.php |
Default implementation returning null when no custom avatar exists |
framework/core/src/Extend/User.php |
Extends User extender with avatarDriver() method for registering custom drivers |
framework/core/src/Api/Resource/UserResource.php |
Adds originalAvatarUrl field to API exposing the raw avatar_url attribute |
framework/core/src/Admin/Content/AdminPayload.php |
Includes available avatar drivers in admin payload for UI configuration |
framework/core/locale/core.yml |
Adds translation keys for avatar driver settings in admin basics page |
framework/core/js/src/forum/components/AvatarEditor.js |
Updates to use originalAvatarUrl() for detecting uploaded avatars |
framework/core/js/src/common/models/User.tsx |
Adds originalAvatarUrl() accessor method to User model |
framework/core/js/src/admin/components/BasicsPage.tsx |
Adds avatar driver selection UI when multiple drivers available |
framework/core/js/src/admin/AdminApplication.tsx |
Adds avatarDrivers to TypeScript interface for admin data |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@imorland, I’ve completed the work on this PR. Should I include the Gravatar driver along with the default driver in the core? |
imorland
left a comment
imorland
left a comment
There was a problem hiding this comment.
Nice one @datlechin this looks good to me.
Yes, let's include gravatar support in core. I think it would be used by some communities, and admins can always choose to switch from 'classic' to gravatar as they wish. I don't think the overhead would be too much here.
I think it best to add that as a seperate PR tho, rather than in this one.
Before I merge this, would you please create an counterpart PR to https://github.com/flarum/docs so that developers can see how to use it?
Documents the new avatar driver system introduced in flarum/framework#4130. Includes implementation guide, examples, and admin configuration.
Documents the new avatar driver system introduced in flarum/framework#4130. Includes implementation guide, examples, and admin configuration.
There was a problem hiding this comment.
Just jumping in here. Generally, I really like the idea of custom avatar drivers and would like to see it for 2.x. I implemented a Gravatar driver for testing purposes locally, and everything is working as expected.
One thing that stood out to me is that two values related to the avatar URL are now serialized (avatarUrl and originalAvatarUrl). I haven’t dug into the details yet as to why this was done, but I generally think we’d be better off serializing just one value to the frontend and letting the backend handle the rest.
Besides this, lgtm
imorland
left a comment
imorland
left a comment
There was a problem hiding this comment.
This is a well-structured feature that follows established Flarum patterns (mirrors displayNameDriver closely). The architecture is sound and it would enable Gravatar, identicons, and similar drivers as first-class extensions. Several things need to be addressed before merge.
Design question: does originalAvatarUrl belong in the public API?
Exposing originalAvatarUrl as a serialised API field is a significant API surface addition. Its only purpose in this PR is to let AvatarEditor know whether the user has a real uploaded avatar (to show the pencil vs plus-circle icon). This is an implementation detail that leaks through to every API consumer. Consider instead returning a boolean field (e.g. hasCustomAvatar) or resolving this client-side by comparing avatarUrl to what a driver would produce. If originalAvatarUrl must be exposed, it should have the same visibility constraints as the existing avatarUrl field.
AvatarEditor.js should be .tsx
The three changed lines in AvatarEditor.js are the only changes to that file. Since this is a JS → TS migration project (#3533), this file could be converted at the same time. Not a blocker, but a missed opportunity.
driverLocale() is not extensible for third-party driver labels
BasicsPage.driverLocale() is a static method that hardcodes the known driver labels. When a Gravatar extension registers an avatarDriver('gravatar', GravatarDriver::class), it has no way to register a human-readable label — it will fall back to the raw identifier string ('gravatar'). The existing displayNameDriver pattern has the same gap, but this PR makes it worse by adding another driver type. Consider documenting this limitation or providing an extension point (e.g. a static avatarDriverLocale map that extensions can populate).
getOriginalAvatarUrlAttribute array access
$this->attributes['avatar_url'] will throw an ErrorException (undefined array key) if the key is not present in the attributes array, e.g. when the model is freshly instantiated without all columns loaded. The existing getAvatarUrlAttribute receives ?string $value = null from Eloquent (which handles absent keys safely). Consider using $this->attributes['avatar_url'] ?? null instead.
Fallback when avatar_driver setting holds an unregistered identifier
In UserServiceProvider, when Arr::get($drivers, $driverName) returns null (unknown driver name in settings), the code falls back to AvatarDefaultDriver. That's reasonable, but it silently ignores a misconfiguration. This matches displayNameDriver behaviour, so acceptable — just worth noting.
No default value registered for avatar_driver setting
The Settings extender is not used to register a default for avatar_driver. Calling $settings->get('avatar_driver', '') with an empty-string default works, but it's inconsistent — an empty string is not the same as 'default'. If a driver named '' were ever registered, this would resolve incorrectly. Using 'default' as the fallback default would be clearer.
Replaces the `originalAvatarUrl` attribute with `hasUploadedAvatar` for a more semantic check of whether a user has uploaded a custom avatar. This change improves clarity and maintainability of avatar-related logic.
Remove unnecessary null check for avatar_url. The `isset()` check already handles cases where the key is not present or its value is null.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
4 participants
This pull request introduces a new avatar driver system. The changes include adding support for different avatar drivers, updating the user model to accommodate these changes, and modifying relevant components to utilize the new avatar driver system.
Fixes #0000
Changes proposed in this pull request:
Reviewers should focus on:
Screenshot
Necessity
Confirmed
composer test).Required changes: