Opened 5 years ago
Last modified 6 days ago
#43515 new enhancement
Twenty Seventeen: Two different function, which return same result
Reported by: |
|
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)
Change History (14)
#3
@
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
@
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();
}
@
3 months ago
makes twentyseventeen_is_static_front_page()
an alias of twentyseventeen_is_frontpage()
#7
follow-up:
↓ 8
@
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.
#8
in reply to:
↑ 7
@
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.
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
@
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
@
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
@
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:
- GitHub code search gives many more results with twentyseventeen_is_frontpage than the results with twentyseventeen_is_static_front_page, which suggests 43515.1.patch would be better.
- The directory search is much closer. The
twentyseventeen_is_frontpage
function is only operational in two of the four plugins in the results list, and a rather popular plugin usestwentyseventeen_is_static_front_page
. Three child themes usetwentyseventeen_is_frontpage
, but the other function's results only include Twenty Seventeen.
Both functions are being used in child themes, etc. we can't just remove them I think: