publishpress / PublishPress-Authors

PublishPress Authors is the best plugin for adding many authors to one WordPress post. You can create multiple authors, co-authors and guest authors.
14 stars 8 forks source link

Rename public methods for better clarity and avoiding name conflicts #520

Closed andergmartins closed 1 year ago

andergmartins commented 3 years ago

The public method get_multiple_authors should be renamed to publishpress_get_post_authors for better clarity and to avoid naming conflicts.

Double-check if that is the better name for it.

We should keep that method as deprecated, and add a setting to load it or not.

The new method should be identical, except for the fact we should remove the argument $filter_the_author_deprecated.

We should update our own code, WP child themes on our Github account, and documentation.

Be careful to not break sites that are currently using it.

andergmartins commented 3 years ago

The following methods should be renamed as well:

andergmartins commented 1 year ago

Hey guys, the motivation for this was a name conflict we had at the time I created this issue. I failed not adding the link or name of the 3rd party plugin/theme on the description, sorry. I know we could simply use a conditional with function_exists before each function, but that would not allow people using both, existent and our function.

Consider a user has a theme which defines the function get_the_authors (that happened before). And he needs to keep that method, but still want to use our version of get_the_authors. How could we do that? Btw, I see we already have the function_exists conditional for most of those functions.

I know a lot of people use these functions right now, that is the reason I suggested moving them to "deprecated" and not remove them.

Some important points. We are not cloning those functions. We should not have repeated code in the plugin (DRY principle).

I still think it worths fixing this issue. We need a better standard for those functions, and need to be careful following the standard when we create new functions for this one or other plugins. Btw, we already did this when we forked the CoAuthors plugin. You will find a similar implementation of my proposal in the file src/functions/coauthors-functions.php: https://github.com/publishpress/PublishPress-Authors/blob/development/src/functions/coauthors-functions.php. There is no code repeated, and all the functions are conditionally loaded, each function, and the file itself (please, check src/functions/template-tags.php:1165: https://github.com/publishpress/PublishPress-Authors/blob/development/src/functions/template-tags.php#L1267-L1269. There is a conditional checking a constant before load the legacy functions.).

Of course we should be careful and do not break sites, so the plan would be:

  1. Copy all those listed functions to a new file src/functions/legacy-functions.php.
  2. Make sure all them are defined inside a function_exists conditional.
  3. Go to the original implementation of each function and rename them with the correct prefix.
  4. Edit each copied function in the legacy-functions file removing the functions' body, and replacing with a call to the specific renamed function.
  5. Add a new conditional to the end of the src/functions/template-tags.php file:

if (PUBLISHPRESS_AUTHORS_LOAD_DEPRECATED_LEGACY_CODE) { require_once legacy-functions.php; }


6. Add the `@deprecated` param to the each function PHPDoc header.
7. Replace all the internal calls to those deprecated functions with the new names.

That constant is already defined as `true` by default. But users that have conflict with any of those legacy function names can define it as `false` in the theme's `functions.php` file, making the plugin to not load them, and only the new functions.

The documentation should be updated to use the new functions names, so new users would be using only the new functions. The changelog should mention all the renamed functions, or point to a documentation page listing them.

And we would probably not delete those deprecated functions, or would wait at least a year for that, in a major release of the plugin, which is expected to pack changes like that in the code. 
andergmartins commented 1 year ago

This approach used in the past, allowed users to use our plugin on themes that support CoAuthors Plus, Byline and Bylines plugins. And will work to support themes/plugins that support the current version of the plugin, changing the code, but having backward compatibility.

andergmartins commented 1 year ago

I would suggest names like:

multiple_authors_get_all_authors -> publishpress_authors_get_all_authors
is_multiple_author_for_post -> publishpress_authors_is_author_for_post
multiple_authors__echo -> publishpress_authors_echo
multiple_authors -> publishpress_authors_the_author

and so on.

ojopaul commented 1 year ago

@stevejburge we'll need documentation link to these function changes as well as changes to their usage in existing documentation if any is already mentioned.

stevejburge commented 1 year ago

@ojopaul Thanks. Are there any specific warnings we should share with users about these changes?

andergmartins commented 1 year ago

@ojopaul just a suggestion, to keep the consistency on the list you sent, maybe the first one could be get_multiple_authors => publishpress_authors_get_post_authors

ojopaul commented 1 year ago

@stevejburge Maybe it's about maintaining a page for deprecated functions?

In the future, when we finally have a large user base, it'll make sense to have developer filters and functions documentation but for now, maintaining a deprecated functions page while showing their alternative will be okay and we can add this to release changelog so it can be referenced for full list of release deprecated functions and their alternative

stevejburge commented 1 year ago

@ojopaul Thanks. Here's the list https://publishpress.com/knowledge-base/deprecated-functions/