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

fix: add naming convention rule #70

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

MaferMazu
Copy link
Contributor

@MaferMazu MaferMazu commented Mar 8, 2024

Description

This PR adds principally a rule for the naming convention prefix check in the phpcs.xml:

<rule ref="WordPress.NamingConventions.PrefixAllGlobals">
	<properties>
		<property name="prefixes" type="array" value="openedx_commerce" />
	</properties>
</rule>

Following the best practice for naming: https://developer.wordpress.org/plugins/plugin-basics/best-practices/#avoid-naming-collisions

The rest of the changes are adapting the namespace to that format.

Testing instructions

This is already uploaded in our stage WordPress installation. You can check if nothing breaks.
Credentials

Additional information

Error from WordPress.org:

Generic function/class/define/namespace/option names

All plugins must have unique function names, namespaces, defines, class and option names. This prevents your plugin from conflicting with other plugins or themes. We need you to update your plugin to use more unique and distinct names.

A good way to do this is with a prefix. For example, if your plugin is called "Easy Custom Post Types" then you could use names like these:
function ecpt_save_post()
define( 'ECPT_LICENSE', true );
class ECPT_Admin{}
namespace ECPT;
update_option( 'ecpt_settings', $settings );

Don't try to use two (2) or three (3) letter prefixes anymore. We host nearly 100-thousand plugins on WordPress.org alone. There are tens of thousands more outside our servers. Believe us, you’re going to run into conflicts.

You also need to avoid the use of __ (double underscores), wp_ , or _ (single underscore) as a prefix. Those are reserved for WordPress itself. You can use them inside your classes, but not as stand-alone function.

Please remember, if you're using _n() or __() for translation, that's fine. We're only talking about functions you've created for your plugin, not the core functions from WordPress. In fact, those core features are why you need to not use those prefixes in your own plugin! You don't want to break WordPress for your users.

Related to this, using if (!function_exists('NAME')) { around all your functions and classes sounds like a great idea until you realize the fatal flaw. If something else has a function with the same name and their code loads first, your plugin will break. Using if-exists should be reserved for shared libraries only.

Remember: Good prefix names are unique and distinct to your plugin. This will help you and the next person in debugging, as well as prevent conflicts.

Analysis result:
This plugin is using the prefix "openedxcommerce" for 7 element(s).
This plugin is using the prefix "openedx" for 8 element(s).

Looks like there are elements not using common prefixes.
openedx-commerce/openedx-commerce.php:41 function activate_openedx_commerce_plugin
openedx-commerce/openedx-commerce.php:50 function deactivate_openedx_commerce_plugin
openedx-commerce/openedx-commerce.php:58 function create_enrollment_logs_table
openedx-commerce/openedx-commerce.php:107 function run_openedx_commerce_plugin

When the report said that we use openedxcommerce in 7 elements as a prefix, it refers to the namespaces. The openedx in 8 elements is for the openedx_commerce in some function names, and it is taking only the first part.

Checklist for Merge

  • Tested in a remote environment
  • Updated documentation
  • Rebased master/main
  • Squashed commits

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 8, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Mar 8, 2024

Thanks for the pull request, @MaferMazu! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@MaferMazu MaferMazu force-pushed the mfmz/naming-prefix branch 4 times, most recently from 6b57360 to 2d831fc Compare April 4, 2024 21:58
@MaferMazu MaferMazu force-pushed the mfmz/naming-prefix branch from 2d831fc to a21db2c Compare April 4, 2024 22:07
@MaferMazu MaferMazu marked this pull request as ready for review April 4, 2024 23:16
Copy link
Member

@felipemontoya felipemontoya left a comment

Choose a reason for hiding this comment

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

This looks great @MaferMazu, the tests are passing with the new check so I believe this is ready to merge.

@MaferMazu MaferMazu merged commit 876ffa8 into openedx:main Apr 5, 2024
5 checks passed
@openedx-webhooks
Copy link

@MaferMazu 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants