-
Notifications
You must be signed in to change notification settings - Fork 30
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
TAXONOMY_NAME to PODCASTING_TAXONOMY_NAME #238
Conversation
New constant name is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but just one note inline. I've asked Darin for a logic check so it can wait until he gets back to us before making the change.
simple-podcasting.php
Outdated
define( 'TAXONOMY_NAME', 'podcasting_podcasts' ); | ||
define( 'PODCASTING_TAXONOMY_NAME', 'podcasting_podcasts' ); | ||
define( 'PODCASTING_ITEMS_PER_PAGE', 250 ); | ||
|
||
trigger_error( 'The constant TAXONOMY_NAME is deprecated and will be removed in future versions. Use PODCASTING_TAXONOMY_NAME instead.', E_USER_DEPRECATED ); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current set up of the plugin doesn't allow site owners to define the constant elsewhere to preempt the value (ie, there is no if ( defined( ... ) )
check). As such I think we can safely hard deprecate the value and only throw a notice if the old value is defined.
cc @dkotter for a logic check.
define( 'TAXONOMY_NAME', 'podcasting_podcasts' ); | |
define( 'PODCASTING_TAXONOMY_NAME', 'podcasting_podcasts' ); | |
define( 'PODCASTING_ITEMS_PER_PAGE', 250 ); | |
trigger_error( 'The constant TAXONOMY_NAME is deprecated and will be removed in future versions. Use PODCASTING_TAXONOMY_NAME instead.', E_USER_DEPRECATED ); | |
define( 'PODCASTING_TAXONOMY_NAME', 'podcasting_podcasts' ); | |
define( 'PODCASTING_ITEMS_PER_PAGE', 250 ); | |
if ( defined( 'TAXONOMY_NAME' ) { | |
$message = sprintf( | |
/* translators: 1: TAXONOMY_NAME, 2: PODCASTING_TAXONOMY_NAME. */ | |
__( 'The %1$s constant is no longer supported. Use %2$s instead.', 'simple-podcasting' ), | |
'TAXONOMY_NAME', | |
'PODCASTING_TAXONOMY_NAME' | |
); | |
trigger_error( wp_strip_all_tags( $message ), E_USER_DEPRECATED ); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, though as mentioned, we didn't really support this value being overridden in the first place so not sure there's a scenario where this deprecation notice would even be used. Doesn't hurt to have it in place though, so I'm fine with this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterwilsoncc
TAXONOMY_NAME
has no prefix specific to Simple Podcasting plugin. What if any other script outside Simple Podcasting plugin uses this constant for their own purpose later? In that case showing deprecation notice might be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thank you!
Review process: Search code for TAXONOMY_NAME
and make sure it it prefixed throughout. It is.
Description of the Change
Constant TAXONOMY_NAME renamed to PODCASTING_TAXONOMY_NAME to avoid potential conflict. Deprecated notice added for older one.
Closes #228
Verification Process
Checklist:
Changelog Entry
Props @jayedul