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

Adding more functions in the discouraged functions list #118

Open
ernilambar opened this issue Jan 23, 2017 · 12 comments
Open

Adding more functions in the discouraged functions list #118

ernilambar opened this issue Jan 23, 2017 · 12 comments

Comments

@ernilambar
Copy link
Member

function name -> recommended usage

  • site_url() -> home_url()
  • get_home_url() -> home_url()
  • paginate_links() -> the_posts_pagination()
  • single_cat_title() -> the_archive_title()
  • single_tag_title() -> the_archive_title()
  • category_description() -> the_archive_description()
  • tag_description() -> the_archive_description()
@grappler
Copy link
Member

Related Theme Check issue WordPress/theme-check#119

I think these would make sense. Just to clarify that these will be warning and not errors.

A PR can be made once we have merged the latest WPCS to this fork which should happen either this week or the next.

@jrfnl
Copy link

jrfnl commented Jan 23, 2017

@grappler Why wait for the back-merge ?
A new WordPress.Theme.AlternativeFunctions or WordPress.Theme.DiscouragedFunctions sniff can be added any time.
As these are quite Theme specific function, that seems most appropriate to me.

@grappler
Copy link
Member

WordPress.Theme.DiscouragedFunctions sounds good. I am not fully sure what I was thinking. Maybe to prevent merge conflicts or that I was high on the parameter abstract sniff.

@ernilambar
Copy link
Member Author

Isnt using home_url() instead of site_url() required? Similarly get_home_url() and paginate_links(). May be we should separate which should be ERROR and which are WARNING.

@grappler
Copy link
Member

Isnt using home_url() instead of site_url() required?

No, but we can always run the sniff on existing themes once is made to see if we can find any use cases.

May be we should separate which should be ERROR and which are WARNING.

The error would be part of restricted functions were as the warnings would be discouraged.

@justintadlock
Copy link

I wouldn't report an error on any of those. There are legit use cases for them in themes. However, more often than not, the functions on the right are more appropriate. So, a warning/info notice might be appropriate.

@joyously
Copy link

joyously commented Apr 9, 2017

How about update_option()? I have had themes change my thumbnail size, my front page choice, created a menu (I know that's another function), and I don't actually know what else. I know they can store their own options, but is there some way to check they are not changing other things?

@grappler
Copy link
Member

grappler commented Apr 9, 2017

@joyously Yes, by checking the parameter/option name that is being modified.

@carolinan
Copy link

Options need to be handled in the customizer,
why would we allow this?
"I know they can store their own options"
What would this be?

@joyously
Copy link

Examples found in themes for site_url() at https://wpdirectory.net/search/01CWPQE2HWN1VCW14NTJZVR73W

$atts['href'] = site_url() . esc_attr( $item->url );

wp_redirect(site_url());

$source = get_site_url();

if (strpos($item->src,get_site_url())!==false):

'domain' => esc_url(site_url()),

$items .= '<li class="menu-item nx-mega-menu"><a href="'. site_url('wp-login.php') .'">

$form = preg_replace( '/<\/form>/i', '<meta itemprop="target" content="' . site_url( '/?s={search} ' ) . '"/></form>', $form );

<a href=\'http://pixelthemestudio.ca/product/pixel-linear/\' target=\'_blank\'><img src='.site_url().'/wp-content/themes/pts-pixel-linear/admin/assets/images/new-ad.jpg /></a> '

echo '<a class="tag" alt="' . $tag->name . '" href="'.site_url("/").'?tag='.$tag->slug.'">'.$tag->name.'</a>';

$siteURL = str_replace( "www.", "", site_url( '/' ) );

@joyously
Copy link

I'm thinking wp_redirect should be on this list.

@dingo-d dingo-d added this to the 0.2.x/0.3.0 Next milestone May 18, 2019
@dingo-d
Copy link
Member

dingo-d commented May 18, 2019

I don't think we have a PR on this, so I'll remove that tag. This sniff can go in a next release (after 0.2.0)

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

7 participants