Some issues with "CHANGE-489, CHANGE-387, CHANGE-341 … improvements to notifier handl

It's not clear (to me, anyway) where issues discovered on the v1.6.0 github repository should be noted (there's no 1.6.0 in the dropdown in the bug reports), so here goes.

Looking through the subject change, there were a couple of places where the notifier call was missing an empty array() parameter:

CHANGE-489, CHANGE-387, CHANGE-341 … improvements to notifier handling

33  includes/classes/order.php
  	1133 	+    $this->notify('NOTIFY_ORDER_INVOICE_CONTENT_READY_TO_SEND', array(), $zf_insert_id, $email_order, $html_msg);

  	1150 	+      $this->notify('NOTIFY_ORDER_INVOICE_CONTENT_FOR_ADDITIONAL_EMAILS', array(), $zf_insert_id, $email_order, $html_msg);

  	1157 	+    $this->notify('NOTIFY_ORDER_AFTER_SEND_ORDER_EMAIL', array(), $zf_insert_id, $email_order, $extra_info, $html_msg);

26  includes/classes/shopping_cart.php
  	507 	+      $this->notify('NOTIFIER_CART_GET_QUANTITY_END_FALSE', array(), $products_id);

  	524 	+      $this->notify('NOTIFIER_CART_IN_CART_END_FALSE', array(), $products_id);

2  includes/functions/functions_email.php
.
  	94 	+        $zco_notifier->notify('NOTIFY_EMAIL_ADDRESS_VALIDATION_FAILURE', array(), sprintf(EMAIL_SEND_FAILED . ' (failed validation)', $to_name, $to_email_address, $email_subject));
     }
6  includes/modules/payment/authorizenet.php
  	498 	+    $this->notify('NOTIFY_PAYMENT_AUTHNETSIM_POSTPROCESS_HOOK', array(), $insert_id);

18  includes/modules/payment/paypalwpp.php
... 	... 	@@ -1578,9 +1578,7 @@ function ec_step1() {
  	2031 	+        $this->notify('NOTIFY_PAYPALEXPRESS_CREATE_ACCOUNT_ADDED_CUSTOMER_RECORD', array(), $customer_id, $sql_data_array);
  	2123 	+        $this->notify('NOTIFY_LOGIN_SUCCESS_VIA_CREATE_ACCOUNT', array(), 'paypal express checkout');

  	2482 	+        $this->notify('NOTIFY_HEADER_ADDRESS_BOOK_ADD_ENTRY_INVALID_ATTEMPT', array(), $customer_id, $country_id, $address_format_id, $address_question_arr);

  	2543 	+    $this->notify('NOTIFY_HEADER_ADDRESS_BOOK_ADD_ENTRY_DONE', array(), 'paypal express checkout', $new_address_book_id, $sql_data_array, $make_default);

Not sure why you believe these should have an empty array as the first parameter.
Nothing forces it to be an array. And various existing observers successfully function with those notifier points passing a non-array element in that position.

Is there a specific problem that's happening? Specific error message? Specific conditions which trigger a particular error?

I dunno:huh:, I guess I just saw a pattern in the changes and questioned why most, but not all, of the changes made for the subject change-requests included an empty array as the second parameter.

Reflecting on it, the addition of that empty array would have the (unwanted) side-effect of causing a v1.6.0 re-write for the vast majority of existing plugins that have adhered to the current notifier/observer interface!

lat9:

Reflecting on it, the addition of that empty array would have the (unwanted) side-effect of causing a v1.6.0 re-write for the vast majority of existing plugins that have adhered to the current notifier/observer interface!Exactly. They were left as-is for backwards-compatibility. And in some cases param1 was repeated again as param2 simply to add the modification ability that params2-8 offer, hence the appearance duplication in some cases.

lat9:

I dunno:huh:, I guess I just saw a pattern in the changes and questioned why most, but not all, of the changes made for the subject change-requests included an empty array as the second parameter.
Good eye :)
The observation is valid, but in this case the pattern isn't a hard-fast-rule.

Nothing wrong with mentioning it though!

DrByte:

Exactly. They were left as-is for backwards-compatibility. And in some cases param1 was repeated again as param2 simply to add the modification ability that params2-8 offer, hence the appearance duplication in some cases.
The majority of the changes to the notifiers in /includes/classes/order.php do not provide backwards-compatibility. For example,

-    $this->notify('NOTIFY_ORDER_AFTER_QUERY', array('order_id' => $order_id));

became

+    $this->notify('NOTIFY_ORDER_AFTER_QUERY', array(), $order_id); 

To provide compatibility, the change should be similar to

+    $this->notify('NOTIFY_ORDER_AFTER_QUERY', array('order_id' => $order_id),  $order_id); 

Noted ... but NOTIFY_ORDER_AFTER_QUERY didn't exist prior to v1.6.0, so there's no backwards-compatibility required.

DrByte:

Noted ... but NOTIFY_ORDER_AFTER_QUERY didn't exist prior to v1.6.0, so there's no backwards-compatibility required.
... other than the fact that it's a notifier that I'd requested for use with a v1.5.x plugin! With the change as-is, I'll need to modify that plugin's update function to be version-specific.

I chose that notifier only as an example; there were many of the notifications within the /includes/classes/order.php file were similarly changed so that they will not be backwards-compatible.