#57293 closed defect (bug) (fixed)
Twenty Twenty-One: Incorrect Mastodon domain (mastodon.com) for Social Icon in Footer Menu
Reported by: | triumvirate | Owned by: | audrasjb |
---|---|---|---|
Milestone: | 6.2 | Priority: | normal |
Severity: | normal | Version: | 5.6 |
Component: | Bundled Theme | Keywords: | has-patch commit has-screenshots |
Focuses: | Cc: |
Description (last modified by )
Reproduce:
Follow this and add a mastodon profile URL
Expected behavior:
adding a link to a Mastodon URL replaces it, like e.g. the other sites here, with the appropriate icon.
Actual behavior: adding a link to a Mastodon instance URL doesn't replace it.
Details:
As there is no entry in social_icons_map
for 'mastodon'
, it defaults to only replacing URLs on the mastodon.com
domain which, while humorous, isn't effective because this isn't a social media website instance, but a Forestry Mulcher Company.
Definition of done:
Adding a link to the following sites results in a Mastodon SVG being used (based on popularity):
mastodon.social pawoo.net mstdn.jp mastodon.cloud mastodon.online counter.social mstdn.social mas.to mastodon.world gc2.jp mastodon.lol pixelfed.social mastodon.sdf.org c.im mastodonapp.uk universeodon.com mastodon.uno masto.ai techhub.social fosstodon.org brighteon.social mastodon.top mstdn.party mamot.fr infosec.exchange m.cmx.im ohai.social sfba.social troet.cafe mastodon.poppingboxes.org hachyderm.io alive.bar home.social kolektiva.social masthead.social
Patch:
https://gist.github.com/hasnolen/e2ded2c7e9b76919c0208ced213e63ab
Attachments (1)
Change History (23)
#1
@
2 years ago
- Description modified (diff)
- Summary changed from Implicit Mastodon domain (mastodon.com) for Social Icon in Footer Menu Incorrect/ Non-Functional to Twenty Twenty-One: Incorrect Mastodon domain (mastodon.com) for Social Icon in Footer Menu
- Version changed from 6.1.1 to 5.6
Thanks for the report!
A similar ticket was opened for Twenty Twenty: #49099.
#2
@
2 years ago
My inclination is to drop automatic Mastodon support and make it manual.
Due to the decentralized nature of the network, keeping up with domains to include will become a never ending battle.
As servers come online and drop offline, the maintenance burden will be substantial so WordPress would need to update the list of domains with every release. There would also need to be a policy document of wehn to support a domain, similar to the policy for adding oembed providers to the allow list.
#3
@
2 years ago
Thanks for taking a look at this!
I haven't done WP development before but have done dev work in general -- if helpful, I'd be more than willing to merge and test this patch, and have the capacity to do so as I'm recently unemployed.
Hm, the domains I listed cover the majority of extant current Mastodon users I believe (it's the top 34 (admittedly an odd number) domains by number of users), which would be far superior than at least the current state of affairs (admittedly, obviously, still more of a maintenance burden than not having Mastodon links at all).
There would also need to be a policy document of wehn to support a domain, similar to the policy for adding oembed providers to the allow list.
Hm, looks like the security concerns with oembed providers is much higher than this, and so oembed might warrant a policy document where this one may not?
#4
@
2 years ago
You're right, there are higher security concerns with oEmbed providers than with including social networks in the block editor and predefined social menus within default themes.
My primary concern forming a policy for Mastodon are about reducing the burden of support by adding and removing servers in each release. The sort of questions I'd ask are:
- How long has the server been active for?
- How many users does the server have, what percentage are active?
- How many toots are sent on a daily basis?
A most-popular servers list is a good start, thank you, but I thinking firming up some criteria is an important initial step as long term contributors are likely to continue maintaining the list.
For context, based on the numbers I found online, Twitter was added as an oembed provider once it passed 100M users. The bar can be lower for social network links, but whether than be 1%, 10% or whatever needs to be decided.
Are you willing to assist with starting a discussion on make.wordpress.org/core -- it's the best spot to form policy such as this?
I know I sound negative, I do support mastodon and like the idea of it a lot! I'm mainly trying to be practical as it increases in popularity.
#5
@
2 years ago
No problem — I totally understand the maintenance concerns, especially as they likely fall on possibly uncompensated open source contributors (such as yourself?) who might have to field inquiries about such and such far-flung instance domain about "why isn't this one supported while this other one is?" far after I myself have walked away satisfied.
Are you willing to assist with starting a discussion on make.wordpress.org/core -- it's the best spot to form policy such as this?
Absolutely!
I can try to enumerate those statistics as well for the 34 I selected above (
How long has the server been active for?
How many users does the server have, what percentage are active?
How many toots are sent on a daily basis?
)
towards that end?
Also, I'm definitely open to workarounds; please let me know if you have a sense of whether this could (should?) be done as an add-on instead of in the theme -- I'm not familiar with what goes where, to be honest.
Relatedly, also, I saw that maybe it's possible to modify functions.php
with existing add-ons? I don't see a way to do that for this, though, to be honest, because looks like we'd basically have to monkey-patch the instance variable of wp-content/themes/twentytwentyone/inc/menu-functions.php::Twenty_Twenty_One_SVG_Icons
which might be private from the perspective of wp-includes/functions.php
? (excuse my Pythonic terminology here, haven't really used PHP).
This ticket was mentioned in Slack in #core by triumvirate. View the logs.
2 years ago
#7
@
2 years ago
The twenty_twenty_one_social_icons_map
filter (in a plugin or child theme) is how to adjust the array now, and it still should work for any domains we do not add in the theme.
<?php function wptrac_tt1_mastodon( $social_icons_map ) { $social_icons_map['mastodon'] = array( 'mastodon.social', 'mastodon.cloud', ); return $social_icons_map; } add_filter( 'twenty_twenty_one_social_icons_map', 'wptrac_tt1_mastodon' );
We should map at least the original Mastodon domain, mastodon.social
, to correct the error. Thoughts about the cutoff:
- Twenty Twenty-One's function checks a total of 49 domains/patterns, so more than 30 domains for the same network is quite a lot.
- The top ten (Mastodon domains) by popularity all record more than 100,000 users, with a significantly lower number for the next.
#8
@
2 years ago
Thanks @sabernhardt. Top 10 domains sounds reasonable as a cutoff to me, as does merely mastodon.social
as well, as I didn't get any traction with the slack question on a policy on the matter. Happy to put through some pull request after I manage to distill the critical bits from the contribution guide.
Thanks for that snippet! @peterwilsoncc suggested the same via Slack; the part that I was missing was the filter API which basically did what I was thinking was impossible in my last paragraph above. Apparently everything is global in PHP or something (??).
Now, I just have to figure out how to replicate this on my toy local dockerized WP setup (got yelled at for testing on prod earlier) which is refusing to render /any/ social URL SVGs whatsoever (even the Facebook etc. ones…)… that's an issue for a separate bug report, though.
#10
@
2 years ago
I'm going to start working on trying to put through a proper pull request for this today (once I divine how to use SVN).
#11
@
2 years ago
Instead of SVN, you could create a PR through GitHub using the wordpress-develop
repository.
This ticket was mentioned in PR #3778 on WordPress/wordpress-develop by ijoseph.
2 years ago
#12
Trac ticket: https://core.trac.wordpress.org/ticket/57293
#13
@
2 years ago
Hm, I was going to add unit tests for this change, but I can't find unit tests for the existing functionality; I looked
- The commit where the SVG icon functionality was first added
- In the
tests/
directory forsvg
tests❯ pwd <snip>/wordpress-develop/tests ❯ rg -l svg phpunit/data/uploads/video-play.svg phpunit/tests/formatting/wpAutop.php phpunit/tests/formatting/emoji.php phpunit/tests/theme/wpThemeJson.php phpunit/tests/dependencies/styles.php phpunit/tests/functions.php
and none of these seemed related.
#14
@
2 years ago
Thanks for the pull request!
I've been thinking about this and @sabernhardt's earlier suggestion of ten is good, otherwise the regex generated by the domains is going to be noticable.
It might be worth setting up a plugin with more known instances using the code above for the filter.
You're right, there are no unit tests for this functionality so I wouldn't worry too much about adding anything additional just yet.
One last thing, are you able to link your GitHub account you're wordpress.org profile, you can link the account on this screen -- linking the accounts will ensure that your contributions are counted correctly.
@triumvirate commented on PR #3778:
2 years ago
#15
@triumvirate commented on PR #3778:
2 years ago
#16
#17
@
2 years ago
Good call with linking.
I tested it manually on local in the Docker image. Sorry for the huge images, I could not, for the life of me, figure out if it's possible to resize images using WikiFormatting so I eventually gave up.
1. Set up Theme: Twenty Twenty-One
2. Create a Secondary Menu
3. Add custom links, bear witness:
- The first link is to Instagram, which is replaced, as expected, with the Instagram SVG logo.
- The second link is to https://mastodon.com, which would have previously, erroneously, been replaced with the Mastodon logo, but now, correctly, is left as-is;
- The third link is to a mastodon.social page, which now, correctly, is replaced by a mastodon logo.
#18
@
2 years ago
Oh ew, sorry for the images above that got auto-posted to trac from GitHub. I was trying to get GitHub to host the images for me so I could insert them in the above post, which didn't work. Ironically, it did it for me but in the way I didn't expect (auto-propagating the Github comments into this ticketing system).
Also, alright, you got me -- I was trying to sneak in more than just 10 as I don't see a huge concern with readability or performance with 10 vs. 40 or so, but I'll respect that proposal and am not going to die on this hill. (I suppose this will mean I'll get to figure out how to make an extension, anyway, for my personal use case (not one of the top 10 :/ ))
I think I managed to link my GitHub -- thanks for the tip!
This ticket was mentioned in Slack in #core by triumvirate. View the logs.
2 years ago
#20
@
2 years ago
- Keywords commit has-screenshots added
- Owner set to audrasjb
- Status changed from new to accepted
Thanks for the PR!
The patch looks good to me. Self assigning for commit
consideration.
@audrasjb commented on PR #3778:
2 years ago
#22
Committed in https://core.trac.wordpress.org/changeset/55061
patch from gist