Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feedback: Unexpected links #31

Open
carolinan opened this issue May 28, 2021 · 9 comments
Open

Feedback: Unexpected links #31

carolinan opened this issue May 28, 2021 · 9 comments

Comments

@carolinan
Copy link
Contributor

carolinan commented May 28, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

carolinan commented May 28, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@StevenDufresne
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

carolinan commented Jun 2, 2021

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
Copy link
Contributor

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

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

@carolinan
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants