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

Resolves phpcs errors #24

Merged
merged 6 commits into from
Jun 7, 2024
Merged

Resolves phpcs errors #24

merged 6 commits into from
Jun 7, 2024

Conversation

nateconley
Copy link
Collaborator

@nateconley nateconley commented Jun 5, 2024

Description of the Change

Edits the plugin to meet phpcs linting rules.

Changes include:

  • Proper escaping and sanitization
  • phpdoc implementations
  • Initializing class variables where applicable
  • json_encode to wp_json_encode
  • Strict comparisons in conditionals and in_array
  • Do not use $var
  • Explicitly set in_footer for scripts
  • Restructure strings to be properly translatable
  • Adds translators: text where placeholders exist
  • Deprecates function where migrated to snake case

Addresses php linting in #12

How to test the Change

  • Ensure plugin functionality is maintained.

Changelog Entry

Deprecated - mailchimpSF_signup_form -> please use mailchimp_sf_signup_form

Credits

@nateconley

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@nateconley nateconley self-assigned this Jun 5, 2024
@github-actions github-actions bot added this to the 1.6.0 milestone Jun 5, 2024
Copy link
Collaborator

@dkotter dkotter left a comment

Choose a reason for hiding this comment

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

A few comments to look at.

The only other thing we need to think through is we're renaming a bunch of functions to avoid snake case. I think this is fine for any functions that are meant to only be used internally but any functions that we expect to be used by others, we need to properly deprecate those so they still work correctly.

As an example, the readme mentions the use of mailchimpSF_signup_form, so I'd imagine there's a decent amount of people using this function on their site. If we don't deprecate this, any site using that function will not only not work but will throw undefined errors.

Not sure what other functions people may be using that we'll want to deprecate. If we're concerned, could also remove the linting rule about snake case and just keep function names as-is, though I like the idea of cleaning those up.

mailchimp_compat.php Outdated Show resolved Hide resolved
mailchimp_compat.php Outdated Show resolved Hide resolved
views/setup_page.php Outdated Show resolved Hide resolved
mailchimp_widget.php Outdated Show resolved Hide resolved
mailchimp_widget.php Outdated Show resolved Hide resolved
mailchimp_widget.php Outdated Show resolved Hide resolved
mailchimp_widget.php Outdated Show resolved Hide resolved
mailchimp_widget.php Outdated Show resolved Hide resolved
mailchimp_widget.php Outdated Show resolved Hide resolved
mailchimp.php Outdated Show resolved Hide resolved
mailchimp_widget.php Outdated Show resolved Hide resolved
lib/mailchimp/mailchimp.php Outdated Show resolved Hide resolved
@nateconley nateconley changed the title Resolves phpcs errors (Draft) Resolves phpcs errors Jun 6, 2024
@nateconley nateconley changed the title (Draft) Resolves phpcs errors Resolves phpcs errors Jun 6, 2024
@nateconley
Copy link
Collaborator Author

@dkotter I updated based on your comments. I used _deprecated_function to deprecate the one function referenced in the documentation and used for display. We could recreate this function if you think that would be more appropriate.

@nateconley nateconley marked this pull request as ready for review June 6, 2024 21:10
@github-actions github-actions bot added the needs:code-review This requires code review. label Jun 6, 2024
@@ -477,29 +500,50 @@ function mailchimp_form_field( $var, $num_fields ) {
/**
* Mailchimp Subscribe Box widget class
*/
class mailchimpSF_Widget extends WP_Widget {
class Mailchimp_SF_Widget extends WP_Widget /* phpcs:ignore Universal.Files.SeparateFunctionsFromOO.Mixed */ {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dkotter Noting that this class probably should live in its own file, but I didn't think this PR was the appropriate time to move it.

@nateconley nateconley requested a review from dkotter June 6, 2024 21:12
@dkotter dkotter merged commit fb30ec9 into develop Jun 7, 2024
6 checks passed
@dkotter dkotter deleted the fix/linting branch June 7, 2024 13:32
@vikrampm1 vikrampm1 mentioned this pull request Jun 26, 2024
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants