Fix a call-time pass-by-reference error

I think this small patch is the complete destroyer of module's functionality.

What this code prints for you?

function myfunc(array &$param) {
  $param['modified'] = TRUE;
}

$params = [
  'modified' => FALSE,
];

call_user_func_array('myfunc', [$params]);

if (empty($params['modified'])) {
  print 'That is how it should not be!';
}
else {
  print 'Works as designed.';
}

And then replace call_user_func_array('myfunc', [$params]); by the call_user_func_array('myfunc', [&$params]);.

Also, did you get a real issue with this? I mean real and not statically generated by some syntax analyzers.

Do not hesitate to reopen this issue if I'm not correct or you have a real issue though.

I agree with #3 this patch breaks things. But I don't know why this functionality works on PHP 7 and not throwing any warnings. By default it should return FALSE and produce many warnings. But one general question here, why call_user_func_array was used here instead of direct call to the function itself: you always know what should be called?

@broken I'm talking about this approach, please take a look at attached patch. In my opinion it will simplify code a bit and it will resolve issue that was originally described in this ticket.

@BAHbKA, the in-use approach is correct because both of thin moments are controlled: function definition and its call. The function defined with the parameter that's handled as a reference. The call of the function via call_user_func_array() simply does what the called function expects - passes value by reference.

Yet another example:

error_reporting(E_ALL);

function call_time_pass_by_reference_CONTROLLED(array &$params): void {
  foreach ($params as $name => $param) {
    if (is_array($param)) {
      call_user_func_array(__FUNCTION__, [&$param]);

      $params[$name] = $param;
    }
  }

  $params['modified'] = TRUE;
}

$data_to_handle = [
  'modified' => FALSE,
  'nested_structure' => [],
];

call_time_pass_by_reference_CONTROLLED($data_to_handle);

if (empty($data_to_handle['modified']) || empty($data_to_handle['nested_structure']['modified'])) {
  print 'That is how it should not be!';
}
else {
  print 'Works as designed.';
}

If you change the definition from call_time_pass_by_reference_CONTROLLED(array &$params): void to call_time_pass_by_reference_CONTROLLED(array $params): void then it'll become uncontolled and you get the warnings/incorrect behavior.

I'm definitely not against the patch you've proposed in a case the https://github.com/wimg/PHPCompatibility has no tags like @codingStandardsIgnoreStart.

Ah, wait, I fooled. The https://github.com/wimg/PHPCompatibility is a coding standard extension for PHPCS, therefore we can use proposed tags.

My proposition.

Fix the incorrect sequence of comments.

Thanks @broken! Initially wanted todo the same but what stopped me is lack of knowledge why it is working.

@BAHbKA, please let me know whether #10 works for you. For now I have no ability to run phpcs.

Well, I managed to run the check and here is what I got:

phpcs --standard=PHPCompatibility docroot/sites/all/modules/ctools_api/

FILE: /var/www/docroot/sites/all/modules/ctools_api/css/form.css
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | WARNING | File appears to be minified and cannot be processed
----------------------------------------------------------------------

Time: 1.27 secs; Memory: 12Mb

Without #10 the result is:

phpcs --standard=PHPCompatibility docroot/sites/all/modules/ctools_api/

FILE: /var/www/docroot/sites/all/modules/ctools_api/css/form.css
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | WARNING | File appears to be minified and cannot be processed
----------------------------------------------------------------------


FILE: ...root/sites/all/modules/ctools_api/includes/ctools_api.common.inc
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 553 | ERROR | Using a call-time pass-by-reference is deprecated
     |       | since PHP 5.3 and prohibited since PHP 5.4
 780 | ERROR | Using a call-time pass-by-reference is deprecated
     |       | since PHP 5.3 and prohibited since PHP 5.4
----------------------------------------------------------------------

Time: 1.26 secs; Memory: 12Mb

Therefore, I'm ready to commit latest patch if you confirm #10 fixes the issue for you.

The final patch that sets up PHPCS (Drupal + PHPCompatibility) checks and PHPUnit tests for the project using Travis CI (PHP 5.6, 7.0 and 7.1 for now).

The latest build can be checked here: https://travis-ci.org/BR0kEN-/ctools_api/jobs/403685217

The new release with all the fixes has been bumped out. Check it out - https://www.drupal.org/project/ctools_api/releases/7.x-1.2.

Thanks everyone!