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

Rewrite WPFunctions methods [MAILPOET-1920] #1896

Merged
merged 4 commits into from
Mar 28, 2019
Merged

Conversation

amine-mp
Copy link
Contributor

@amine-mp amine-mp commented Mar 20, 2019

I rewrote the whole WP/Functions.php file and sorted methods alphabetically.
To review, please read the file as if it's a new one, because the diff doesn't make sense due to the reordering.

@amine-mp amine-mp force-pushed the rewrite-wp-functions branch 7 times, most recently from 57a6303 to cd6ed76 Compare March 21, 2019 17:13
@amine-mp amine-mp requested a review from costasovo March 21, 2019 17:38
Copy link
Contributor

@costasovo costasovo left a comment

Choose a reason for hiding this comment

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

I like the change since IDE will give me parameter hints.
Just want to ask why did you kept firs 6 functions called using call_user_func_array ?

@costasovo costasovo assigned amine-mp and unassigned costasovo Mar 25, 2019
@amine-mp
Copy link
Contributor Author

I kept using call_user_func_array for WP functions which have ambiguous arguments (either accept variables number of args or have optional arguments in the middle).
I also used it for WC functions because they are not always defined and PHPStan was not happy with them even when I added if(function_exists(..)) before calling them.

@amine-mp amine-mp assigned costasovo and unassigned amine-mp Mar 25, 2019
Copy link
Contributor

@costasovo costasovo left a comment

Choose a reason for hiding this comment

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

I did not realized that some functions are from wooCommerce during the first review. I think we should move those to their own wrapper.

}

function wpRemoteRetrieveResponseCode() {
return call_user_func_array('wp_remote_retrieve_response_code', func_get_args());
function wcGetCustomerOrderCount($user_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking about WooCommerce functions. We already have a service WooCommerce\Helper
so I what about putting woo commerce functions into this helper? They are not WordPress functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

PHP Stan doesn't support function_exist check :( but the author mentions here that we can use is_callable as a workaround.

}

function wpRemoteRetrieveBody() {
return call_user_func_array('wp_remote_retrieve_body', func_get_args());
function addAction() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add action has not dynamic number of arguments so I think this one can be refactored

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I refactor addAction tests start failing and I don't know why, I kept it that way.

@@ -19,486 +19,499 @@ static function set(Functions $instance) {
self::$instance = $instance;
}

function wpRemotePost() {
return call_user_func_array('wp_remote_post', func_get_args());
function doAction() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that for this and applyFilters we could do at least this:

  /**
   * @param string $tag
   * @param mixed ...$args
   * @return mixed
   */
  function doAction($tag, ...$args) {
...
}

@costasovo costasovo assigned amine-mp and unassigned costasovo Mar 25, 2019
@amine-mp amine-mp force-pushed the rewrite-wp-functions branch 2 times, most recently from eb95d41 to c68cee5 Compare March 27, 2019 16:50
Copy link
Contributor

@costasovo costasovo left a comment

Choose a reason for hiding this comment

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

👍

@costasovo costasovo removed their assignment Mar 28, 2019
@michelleshull michelleshull merged commit 6f85c27 into master Mar 28, 2019
@michelleshull michelleshull deleted the rewrite-wp-functions branch March 28, 2019 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants