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

TAXONOMY_NAME to PODCASTING_TAXONOMY_NAME #238

Merged
merged 3 commits into from Jul 2, 2023
Merged

TAXONOMY_NAME to PODCASTING_TAXONOMY_NAME #238

merged 3 commits into from Jul 2, 2023

Conversation

jayedul
Copy link
Contributor

@jayedul jayedul commented Jun 13, 2023

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

  • All features are working as usual.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Changelog Entry

Props @jayedul

@jayedul jayedul self-assigned this Jun 13, 2023
@jayedul
Copy link
Contributor Author

jayedul commented Jun 13, 2023

New constant name is PODCASTING_TAXONOMY_NAME to keep consistency with others, though suggested name was SIMPLE_PODCASTING_TAXONOMY_NAME.

@jayedul jayedul marked this pull request as ready for review June 13, 2023 09:56
@jayedul jayedul requested a review from a team as a code owner June 13, 2023 09:56
@jayedul jayedul requested review from iamdharmesh and removed request for a team June 13, 2023 09:56
@jeffpaul jeffpaul added this to the 1.5.0 milestone Jun 13, 2023
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a 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.

Comment on lines 21 to 26
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 );

Copy link
Contributor

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.

Suggested change
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 );
}

Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkotter @jayedul That makes sense, let's just replace the constant without a deprecation notice.

@jeffpaul jeffpaul modified the milestones: 1.5.0, 1.6.0 Jun 28, 2023
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a 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.

@peterwilsoncc peterwilsoncc merged commit ad10ed3 into develop Jul 2, 2023
8 checks passed
@peterwilsoncc peterwilsoncc deleted the fix/228 branch July 2, 2023 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TAXONOMY_NAME constant name is going to lead to a conflict.
4 participants