wpsharks / alert-box-shortcode

Alert Box Shortcode for WordPress by WP Sharks™.
https://wpsharks.com/
GNU General Public License v3.0
0 stars 0 forks source link

Use proper naming standards in codebase; See: websharks/alert-box-shortcode#2 #6

Closed kristineds closed 8 years ago

kristineds commented 8 years ago

Use proper naming standards;


Description: Provides a [shortcode] shortcode.

screen shot 2016-05-06 at 12 49 20 am

See: websharks/alert-box-shortcode#2

kristineds commented 8 years ago

@jaswsinc @raamdev Submitting my Pull Request. https://github.com/websharks/alert-box-shortcode/pull/6/commits/c9e3bdc50d9d34c1ca50cd42ee350b8eaa30c85c

I have commented out the line for declaring the strict types as it causes the shortcode to not work when I add it. Any ideas why? :)

declare (strict_types = 1);
namespace WebSharks\WpSharks\AlertBoxShortcode\Classes;
raamdev commented 8 years ago

Any particular reason you're adding declare (strict_types = 1);? Isn't that a PHP7-only feature?

jaswrks commented 8 years ago

Lookin' great! :-) Here are some observations:


@kristineds writes...

I have commented out the line for declaring the strict types as it causes the shortcode to not work when I add it. Any ideas why? :)

After the above changes, if you're still having trouble let me know what error the strict types declaration causes and I will help you troubleshoot the issue. Strict types are always better to have enabled when you're writing a PHP v7+ codebase, as it enforces strict types for all function parameters; i.e., it will raise an exception if you are passing the wrong data type between functions, which would otherwise go unchecked with strict types off; and that can lead to unreliable and unpredictable behavior.

@raamdev writes...

Any particular reason you're adding declare (strict_types = 1);? Isn't that a PHP7-only feature?

I think she was following the suggestion that I made to add strict types for compatibility with the upcoming release of the WP Sharks Core. The Alert Box Shortcode plugin doesn't necessarily need to be written in a way that conforms to the standards I'll be using in WPSC projects (i.e., this is a simple shortcode plugin), but as a learning exercise I was happy to see that she added it here.

All code that I write over the next few years will be designed to run on PHP v7.0.4 (or higher). The WebSharks Core, the WP Sharks Core, s2Member X, our websites, and several other libraries that are coming together to form the foundation of wpsharks.com are going to be written with the PHP v7.0.4 minimum requirement. So becoming familiar with strict types in PHP v7 will be very helpful.

In PHP, strict types prevent bugs caused by unexpected behavior. I have been forcing strict types in my code for several years now, but with PHP 7 we can enforce this at the interpreter level, and PHP v7 also enables the use of type hints for primitives that was previously not possible—making strict types a much more effective way to prevent problems associated with loose type comparison. See: http://php.net/manual/en/functions.arguments.php#functions.arguments.type-declaration.strict

jaswrks commented 8 years ago

With strict types enabled, most function parameters should be type-hinted also.

public function shortcode($attr, $content, $apply_shortcode)

Should become:

public function shortcode(array $attr, string $content = null, string $apply_shortcode = '')
jaswrks commented 8 years ago

There should also be a return-type declaration on the shortcode() method.

What we have thus far, is:

public function shortcode(array $attr, string $content = null, string $apply_shortcode = '') {

This should become:

public function shortcode(array $attr, string $content = null, string $apply_shortcode = ''): string {

Note that ): string { forces a strict return-type of string; i.e., the function MUST return a string, which is exactly what any caller would expect to receive from this function. We add the hint so that any changes to the code that might return anything other than a string will raise an exception and bring it to our attention quickly.

kristineds commented 8 years ago

@jaswsinc @raamdev Submitting my PR for this: https://github.com/websharks/alert-box-shortcode/pull/6/commits/d2a9f01edeba8ffbaa362226d538aa4e4c8289c9

screen shot 2016-05-16 at 2 07 07 am

kristineds commented 8 years ago

@jaswsinc @raamdev Any feedback on this? 😄 So we could start renaming the 000000-dev branch to dev as mentioned here. https://github.com/websharks/alert-box-shortcode/issues/7

jaswrks commented 8 years ago

@kristineds This looks great to me!

kristineds commented 8 years ago

@raamdev Should I merge this into 000000-dev? 😃

jaswrks commented 8 years ago

@kristineds Raam's going to be away on Friday so I thought I would try to help by responding here. Yep, I'd go ahead and do the merge and then you can rename the branch as outlined here under 'Attn: WebSharks Project Leads'. https://github.com/websharks/phings/issues/124

kristineds commented 8 years ago

Merged and closed! Thank you @jaswsinc and @raamdev! 😃