theme-review-action icon indicating copy to clipboard operation
theme-review-action copied to clipboard

Feedback: Unexpected links

Open carolinan opened this issue 4 years ago • 9 comments

Unexpected links

It is not 100% clear from the report or docs which requirement it refers to. I believe it is this one? Themes may have a single footer credit link, which is restricted to the Theme URI or Author URI defined in style.css

Is it possible to include an explanation in the report, that the link is not approved because it is not also listed as the theme or author URI? I do not understand the code well enough to see where this is confirmed.


Example report: https://themes.trac.wordpress.org/ticket/97704#comment:10 At the point of the report, theme did not have a theme or author URI but credit links:

themearrow.com found on /?p=1241 is not an approved link.
See: https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md#page-should-not-have-unexpected-links
themearrow.com found on / is not an approved link.
See: https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md#page-should-not-have-unexpected-links
themearrow.com found on /?cat=1 is not an approved link.
See: https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md#page-should-not-have-unexpected-links
themearrow.com found on /?tag=alignment-2 is not an approved link.
See: https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md#page-should-not-have-unexpected-links
/?post_format=gallery contains PHP errors: Notice: Undefined index: with_front in wp-content/themes/test-theme/inc/breadcrumbs.php on line 529
See: https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md#page-should-not-have-php-errors
themearrow.com found on /?post_format=gallery is not an approved link.
See: https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md#page-should-not-have-unexpected-links

I would also suggest that the requirement itself is changed to Themes can include one single front facing credit link, which is restricted to the Theme URI or Author URI defined in style.css. I will bring that up with the themes team.

carolinan avatar May 28 '21 06:05 carolinan

Another example report, the theme has theme/author URI but the credit link does not match https://themes.trac.wordpress.org/ticket/100017#comment:1

carolinan avatar May 28 '21 06:05 carolinan

I am wondering how to best handle the list of social media links.

Whatsapp is not listed in https://github.com/WordPress/theme-review-action/blob/trunk/actions/ui-check/tests/e2e/specs/page/unexpected-links/index.js#L19

Whatsapp: https://themes.trac.wordpress.org/ticket/99968#comment:2 Telegram: https://themes.trac.wordpress.org/ticket/99570#comment:2 Vimeo: https://themes.trac.wordpress.org/ticket/99435#comment:2 Instagram: + /?feed=rss2 https://themes.trac.wordpress.org/ticket/98765

Wordpress.com Are there any other reasons for reducing tellyworth.wordpress.com to just wordpress.com?

<?php
/* translators: %s: CMS name, i.e. WordPress. */
esc_html_e('Powered by', 'estera'); ?>
<a href="<?php echo esc_url( __('https://wordpress.com/', 'estera' ) ); ?>" class="imprint">
<?php esc_html_e ( 'WordPress', 'estera' ); ?>
</a>

https://themes.trac.wordpress.org/ticket/97852

GitHub. You can blame @aristath for this one. https://themes.trac.wordpress.org/ticket/97559#comment:2

carolinan avatar May 28 '21 06:05 carolinan

This report is for a block theme, so we can be assured it works for them 👍 https://themes.trac.wordpress.org/ticket/99906#comment:2

carolinan avatar May 28 '21 06:05 carolinan

This is a child theme, this seems like a false positive: https://themes.trac.wordpress.org/ticket/98667#comment:7

carolinan avatar May 28 '21 07:05 carolinan

This is a child theme, this seems like a false positive: https://themes.trac.wordpress.org/ticket/98667#comment:7

No, this is a problem with the parent theme:

printf( esc_html__( ' BlogJr by %1$s Shark Themes %2$s', 'blogjr' ), '<a href="' . esc_url( 'https://sharkthemes/' ) . '" target="_blank">','</a>' );

StevenDufresne avatar Jun 01 '21 02:06 StevenDufresne

I am wondering how to best handle the list of social media links.

Whatsapp is not listed in https://github.com/WordPress/theme-review-action/blob/trunk/actions/ui-check/tests/e2e/specs/page/unexpected-links/index.js#L19

Whatsapp: https://themes.trac.wordpress.org/ticket/99968#comment:2 Telegram: https://themes.trac.wordpress.org/ticket/99570#comment:2 Vimeo: https://themes.trac.wordpress.org/ticket/99435#comment:2 Instagram: + /?feed=rss2 https://themes.trac.wordpress.org/ticket/98765

Are you suggesting we whitelist these for now?

Wordpress.com Are there any other reasons for reducing tellyworth.wordpress.com to just wordpress.com?

<?php
/* translators: %s: CMS name, i.e. WordPress. */
esc_html_e('Powered by', 'estera'); ?>
<a href="<?php echo esc_url( __('https://wordpress.com/', 'estera' ) ); ?>" class="imprint">
<?php esc_html_e ( 'WordPress', 'estera' ); ?>
</a>

https://themes.trac.wordpress.org/ticket/97852

Adding @tellyworth since he's may be more familiar with the reasoning...

StevenDufresne avatar Jun 01 '21 02:06 StevenDufresne

Yes. These links should be allowed because simple social sharing links are allowed. It's not great to maintain an allowed-list but I can't think of better options.

carolinan avatar Jun 02 '21 10:06 carolinan

Yes. These links should be allowed because simple social sharing links are allowed. It's not great to maintain an allowed-list but I can't think of better options.

I've added the links in https://github.com/WordPress/theme-review-action/commit/58ca1799e8d092d630481b3d1a51d5be334160f1.

We still need a better approach for Theme URI/Author URIs so i'll leave this ticket open.

StevenDufresne avatar Jun 03 '21 06:06 StevenDufresne

Does this also check for "No remote resources are allowed. Include all scripts, images, videos and other resources rather than hot-linking."?

I mean if there is an allowed list, then the CDN check is not needed? https://github.com/WordPress/theme-check/blob/master/checks/class-cdn-check.php

carolinan avatar Jun 10 '21 10:06 carolinan