Make WordPress Core

Opened 6 years ago

Closed 10 months ago

#43515 closed enhancement (fixed)

Twenty Seventeen: Two different function, which return same result

Reported by: mukesh27's profile mukesh27 Owned by: audrasjb's profile audrasjb
Milestone: 6.3 Priority: low
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch commit
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 6 years ago.
43515.1.patch (1.0 KB) - added by sabernhardt 14 months ago.
makes twentyseventeen_is_static_front_page() an alias of twentyseventeen_is_frontpage()

Download all attachments as: .zip

Change History (20)

@mukesh27
6 years ago

#1 @mukesh27
6 years ago

  • Keywords has-patch added
  • Version 4.9.4 deleted

#3 @mukesh27
6 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.


6 years ago

#5 @swissspidy
6 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
6 years ago

  • Keywords dev-feedback added; needs-patch removed

@sabernhardt
14 months ago

makes twentyseventeen_is_static_front_page() an alias of twentyseventeen_is_frontpage()

#7 follow-up: @sabernhardt
14 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
12 months 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
12 months 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
11 months 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
11 months 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.


11 months ago

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


11 months ago

#14 @oglekler
10 months ago

  • Keywords needs-refresh added

#15 @audrasjb
10 months ago

  • Keywords needs-refresh removed

I think both approaches would work. Given the theme is not in active development anymore, I'd suggest to go with 43515.1.patch so we can ship this before 6.3 beta 1. Thoughts?

#16 @Hareesh Pillai
10 months ago

43515.1.patch looks good.
This approach seems better, considering the amount of changes needed in both the approaches.

#17 @sabernhardt
10 months ago

  • Keywords commit added

Marking for commit then.

#18 @audrasjb
10 months ago

  • Owner set to audrasjb
  • Resolution set to fixed
  • Status changed from new to closed

In 56012:

Twenty Seventeen: Make twentyseventeen_is_static_front_page() an alias of twentyseventeen_is_frontpage().

Since both functions return the exact same thing, this changeset makes twentyseventeen_is_static_front_page() an alias of twentyseventeen_is_frontpage()
and updates the related docblocks and function callback.

Props mukesh27, soulseekah, swissspidy, sabernhardt, audrasjb, SergeyBiryukov, hareesh-pillai.
Fixes #43515.

Note: See TracTickets for help on using tickets.