Skip to content

Splitting out functions from main file #539

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

Closed
wants to merge 310 commits into from
Closed

Splitting out functions from main file #539

wants to merge 310 commits into from

Conversation

TheCrowned
Copy link
Contributor

This fixes #396. The main CAP file was unnecessarily big, and some functions could be taken out of it to reduce its size.

I have done that basically following the plan I had outlined in #396. All the functions that I moved are now in standalone files (i.e. not wrapped in classes) and prefixed with cap_. I have updated all relevant hooks as well, of course!

In some cases, I have taken the chance to update the functions inline documentation and also make them adherent to our coding standards.

New files are:

  • php/coauthors-edit-screen.php - Functions that change the default behavior of the edit.php page.
  • php/coauthors-users-screen.php - Functions that change the default behavior of the users.php page.
  • php/coauthors-meta-box.php - Functions related to metabox handling.
  • php/integrations/edit-flow.php - Functions related to Edit Flow integration.
  • php/integrations/jetpack.php - Functions related to Jetpack integration.

All functions have been moved out (i.e. deleted) from the main file, except the metabox ones, for which I have left helper functions where they were before, so that any external code reference to those would not break.

I skipped the planned php/email-notifications.php one since those functions have actually been (or are going to be?) removed as per PR #421. Those may me moved, although they would be a small change, when the relevant PR is merged.

Before splitting, I created a bunch of new unit tests for the functions I was going to move. Since my new tests were covering functions I had moved out of the main file, I moved the tests out of the main test file as well, resulting in the creation of:

  • tests/test-edit-screen.php
  • tests/test-integration-edit-flow.php
  • tests/test-users-screen.php

All unit tests worked before the split, and they still work afterwards. I have tested CAP functionalities (at least, the normal/basic ones, if that makes sense) and I found nothing broke as a result of my changes.

As a final note, although this look like a huge change set, most of the changes are just copy-pasting code around, so there was little room for breaking.

Chirag Patel and others added 30 commits May 24, 2017 18:01
Script was stopping after no co-authors was find. Now script will
display an error message and continue to the next post.
Script was stopping after no co-authors was find. Now script will
display an error message and continue to the next post.
Resolves #417

* Removed duplicate left join for optimization
Fixed WP CLI create-terms-for-posts if no co-authors found
Terminology updated throughout
Replace hardcoded 'author' with $this->$coauthor_taxonomy
Fixes warning: `Warning: sprintf(): Too few arguments in /path/wp-content/plugins/co-authors-plus/php/class-coauthors-guest-authors.php on line 487`

Warning surfaces when deleting a guest author that is mapped to a WP user
TheCrowned and others added 14 commits June 7, 2018 10:10
…thors_Plus::_filter_manage_users_custom_column.
…s, CoAuthors_Plus::_filter_manage_posts_custom_column, CoAuthors_Plus::filter_views.
… had been splitted out.

Updating tests with new functions names due to main file splitting. All tests are working!
* [Style] Identation changed to tab

* [Feat] Added personal data exporter to guest authors

* [Feat] Added a filter to allow third part plugins add guest author data

* [Style] WPCS fixes

* [Fix] Filter name changed

* [Style] WPCS fixes

* [Fix] Filter name changed
Copy link

@emrikol emrikol left a comment

Choose a reason for hiding this comment

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

A tiny bit of feedback on the code. I don't have any specific insights on the architectural changes, and I'll leave that up to @philipjohn :)

@@ -314,221 +319,30 @@ public function is_post_type_enabled( $post_type = null ) {
}

/**
* DEPRECATED - Use cap_remove_authors_box() instead!
Copy link

Choose a reason for hiding this comment

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

For these deprecated functions, can we call _deprecated_function() to throw a warning and add a @deprecated and/or @see tag (Kinda like https://github.com/WordPress/WordPress/blob/505c2ddb61b1509c49bd0d9e8328c6a1856f6827/wp-admin/includes/deprecated.php#L97)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure - will update that! What version should I use in the deprecated notice?

_deprecated_function( __FUNCTION__, '??????', 'cap_remove_authors_box()' );

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets use 3.3.0 as the version as I don't think we've nailed down where this will be integrated to the code can be updated again when it is merged if needed to align to the latest version number.

@mdbitz mdbitz requested a review from philipjohn June 25, 2018 15:35
@mdbitz
Copy link
Contributor

mdbitz commented Jun 25, 2018

@philipjohn I'm adding you as a review for this. Thanks in advance!

rebeccahum and others added 2 commits June 25, 2018 10:35
🐛Only filter author template for title
@GaryJones
Copy link
Contributor

While I'm greatly in favour of organising the codebase, and someone could use the commits here as ideas and inspiration for how to do it, it's clear that this PR is so far behind and conflicted that it would be significantly easier to create fresh PRs with much smaller iterative changes. As such, I'm going to close this one out, but not because of the tremendous amount of effort that had gone into it!

@GaryJones GaryJones closed this Jul 27, 2023
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.

Separate out some functions