Make WordPress Core

Opened 5 years ago

Last modified 6 days ago

#43515 new enhancement

Twenty Seventeen: Two different function, which return same result

Reported by: mukesh27's profile mukesh27 Owned by:
Milestone: 6.3 Priority: low
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:

Description

Many apologies if this is a duplicate. I have searched but did not find it yet posted.

Theme: Twenty Seventeen
Version: 1.4 (not tested below 1.4)

The theme have two different functions twentyseventeen_is_frontpage and twentyseventeen_is_static_front_page both have return same result so why we create two different function as both function return same result? They have to remove "twentyseventeen_is_static_front_page" function use in one place.

Attachments (2)

43515.patch (924 bytes) - added by mukesh27 5 years ago.
43515.1.patch (1.0 KB) - added by sabernhardt 3 months ago.
makes twentyseventeen_is_static_front_page() an alias of twentyseventeen_is_frontpage()

Download all attachments as: .zip

Change History (14)

@mukesh27
5 years ago

#1 @mukesh27
5 years ago

  • Keywords has-patch added
  • Version 4.9.4 deleted

#3 @mukesh27
5 years ago

  • Keywords dev-feedback added

@soulseekah above both link are not working.

My question is below both the functions return same result then wy should we use two different function?

twentyseventeen_is_static_front_page function

<?php
function twentyseventeen_is_static_front_page() {
   return ( is_front_page() && ! is_home() );
}

twentyseventeen_is_frontpage function

<?php
function twentyseventeen_is_frontpage() {
   return ( is_front_page() && ! is_home() );
}

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


5 years ago

#5 @swissspidy
5 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed
  • Priority changed from normal to low

@mukesh27 You need to be logged in on GitHub in order for the links to work.

This should rather look something like this (or the other way around):

function twentyseventeen_is_frontpage() {
  return twentyseventeen_is_static_front_page();
}

#6 @mukesh27
5 years ago

  • Keywords dev-feedback added; needs-patch removed

@sabernhardt
3 months ago

makes twentyseventeen_is_static_front_page() an alias of twentyseventeen_is_frontpage()

#7 follow-up: @sabernhardt
3 months ago

  • Milestone changed from Awaiting Review to 6.3

I think the name twentyseventeen_is_static_front_page() is more self-explanatory, but the other function is much more common and it was created first.

  • [38833] imported the theme from GitHub with twentyseventeen_is_frontpage().
  • [38986] added twentyseventeen_is_static_front_page(), still before version 1.0.

#8 in reply to: ↑ 7 @audrasjb
7 weeks ago

  • Keywords dev-feedback removed

Replying to sabernhardt:

I think the name twentyseventeen_is_static_front_page() is more self-explanatory, but the other function is much more common and it was created first.

  • [38833] imported the theme from GitHub with twentyseventeen_is_frontpage().
  • [38986] added twentyseventeen_is_static_front_page(), still before version 1.0.

Given both were added before version 1.0, I think we can safely go with twentyseventeen_is_static_front_page() which is indeed more self-explanatory 👍

#9 @SergeyBiryukov
7 weeks ago

  • Keywords has-patch added

I think 43515.1.patch looks good.

twentyseventeen_is_frontpage() is indeed more common and used in other parts of the theme, while twentyseventeen_is_static_front_page() was only ever used as a Customizer control callback.

Since the theme is no longer in active development, making the latter an alias of the former would minimize the changes, otherwise I think we'd have to update all instances of twentyseventeen_is_frontpage() in the theme.

#10 @Hareesh Pillai
9 days ago

Just to be sure - I hope we don't need to handle this in the doing_it_wrong or deprecated.php file?

#11 @sabernhardt
6 days ago

I think both functions would remain equally valid, but with one canonical.

To consider switching it the other way, I drafted a PR that edits multiple files to use the twentyseventeen_is_static_front_page function throughout the theme (and it includes two unrelated documentation changes that probably belong on #57840 instead). If that direction might be preferable, I could move it from my fork to the main repo for review.

Code outside Twenty Seventeen:

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


6 days ago

Note: See TracTickets for help on using tickets.