Allowing user to specify full URL to social page in widget#6016
Allowing user to specify full URL to social page in widget#6016howtoaddict wants to merge 2 commits intoAutomattic:masterfrom
Conversation
Previously user could only specify username on Social media. This caused problems with some sites like Youtube... on which you can have both User (http://youtube.com/u/Username) and Channel (http://youtube.com/c/Channel).
|
Fixes #6010 |
eliorivero
left a comment
There was a problem hiding this comment.
Thanks @howtoaddict for the work on this. This works well and it's a very necessary addition.
I'm blocking this for now mainly due to the translation issue but the other one should be addressed too for more concise code.
Thank you!
| $html[ $index ] = | ||
| '<a href="' . $username | ||
| . '" class="genericon genericon-' . $service . '" target="_blank"><span class="screen-reader-text">' | ||
| . 'View our profile on '. $service_name |
There was a problem hiding this comment.
This text must be made available for translation like this:
/* Translators: the placeholder is a social network name. */
. esc_html( sprintf( __( 'View our profile on %s', 'jetpack' ), $service_name ) )
|
|
||
| /** Check if full URL entered in configuration, use it instead of tinkering **/ | ||
| if ( substr( $username, 0, 5 ) === "http:" | ||
| || substr( $username, 0, 6 ) === "https:" |
There was a problem hiding this comment.
This should be replaced with
if ( preg_match( '#^https?://#', $username ) ) {
|
@eliorivero updated |
|
Would this change let someone paste in a link to "http://hackmysite.tld" and have it show with the social media icon of the relevant service? I'm not sure if it may be wise to build in some sort of domain name verification, or if that's being overly and unnecessarily cautious. |
|
Yeah, George is right. We shouldn't allow people to do evil. Maybe something like this would be enough? is_valid_social_network( $url ) {
return in_array( parse_url( $url, PHP_URL_HOST ), array(
'twitter.com',
'facebook.com',
etc etc
) );
} |
|
@georgestephanis @eliorivero yeah... like you can't use other components in Jetpack to do exactly that - put COMPLETELY custom HTML. I've invested time trying to help and this is obviously beneficial and requested feature. If you guys don't want to add it don't merge it. I'm giving up on this pull request. |
|
Right, @howtoaddict has a very valid point. There are many many places throughout WordPress core and Jetpack features that allow inserting custom HTML. Someone could do the same thing with a simple WordPress text widget. I don't think it's very realistic to expect us to filter all of these instances. @eliorivero's suggestion does seem pretty reasonable in theory, but that would require us having to create an index of full/shorthand/custom social media URLs that are used everywhere. IMO more effort than it's worth. |
|
Let's 🐑 this. Thanks @howtoaddict 👍 |
|
Closing this in favour of #6508, thanks for the idea and the implementation! |
* Allowing user to specify full URL to social page in widget Previously user could only specify username on Social media. This caused problems with some sites like Youtube... on which you can have both User (http://youtube.com/u/Username) and Channel (http://youtube.com/c/Channel). * Changes resulting from code review #6016 (review) * Sanitized URLs, fixed screen reader text for predefined URLs.
Previously user could only specify username on Social media. This caused
problems with some sites like Youtube... on which you can have both User
(http://youtube.com/u/Username) and Channel
(http://youtube.com/c/Channel).