Feedback: Unexpected links

carolinan commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

StevenDufresne commented 3 years ago

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 commented 3 years ago

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...

carolinan commented 3 years ago

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.

StevenDufresne commented 3 years ago

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.

carolinan commented 3 years ago

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

WordPress / theme-review-action

Feedback: Unexpected links #31

Unexpected links