Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#57293 closed defect (bug) (fixed)

Twenty Twenty-One: Incorrect Mastodon domain (mastodon.com) for Social Icon in Footer Menu

Reported by: triumvirate's profile triumvirate Owned by: audrasjb's profile 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 sabernhardt)

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)

add_mast_urls.diff (1.2 KB) - added by sabernhardt 2 years ago.
patch from gist

Download all attachments as: .zip

Change History (23)

@sabernhardt
2 years ago

patch from gist

#1 @sabernhardt
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 @peterwilsoncc
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 @triumvirate
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?

Last edited 2 years ago by triumvirate (previous) (diff)

#4 @peterwilsoncc
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 @triumvirate
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 @sabernhardt
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.
Last edited 2 years ago by sabernhardt (previous) (diff)

#8 @triumvirate
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.

#9 @sabernhardt
2 years ago

  • Milestone changed from Awaiting Review to 6.2

#10 @triumvirate
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 @sabernhardt
2 years ago

Instead of SVN, you could create a PR through GitHub using the wordpress-develop repository.

#13 @triumvirate
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

  1. The commit where the SVG icon functionality was first added
  2. In the tests/ directory for svg 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 @peterwilsoncc
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.

#17 @triumvirate
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

https://i.postimg.cc/qvWwpNxB/19-E91-A577-C9121-B2-FDBC456-BD89-E6684.jpg

2. Create a Secondary Menu

https://i.postimg.cc/gjcqg8ms/21-DB747-FF9044-F2-E1-E00163914-E0806-F.jpg

3. Add custom links, bear witness:

  1. The first link is to Instagram, which is replaced, as expected, with the Instagram SVG logo.
  2. 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;
  3. The third link is to a mastodon.social page, which now, correctly, is replaced by a mastodon logo.

https://i.postimg.cc/NfMkT6Nf/61-A2566-B19-C7-EA463-F6-FD24-B0960-D075.jpg

https://i.postimg.cc/Kj97wjY2/F7-B20709-C2-DE59-ABD073-D966-B26371-B2.jpg

#18 @triumvirate
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!

Last edited 2 years ago by triumvirate (previous) (diff)

This ticket was mentioned in Slack in #core by triumvirate. View the logs.


2 years ago

#20 @audrasjb
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.

#21 @audrasjb
2 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 55061:

Bundled Themes: Add Mastodon domains for menu item icons.

This changeset adds the top 10 Mastodon domains by popularity to Twenty Twenty and Twenty Twenty-One Social icon feature.

Note: other Mastodon domains can be added to the array of domain mapping using twentytwenty_social_icons_map and twenty_twenty_one_social_icons_map filters.

Props triumvirate, sabernhardt, audrasjb, peterwilsoncc, rryyaanndd, ianbelanger.
Fixes #57293, #49099.

Note: See TracTickets for help on using tickets.