On account of the backwards-incompatible changes made in PHP 7.0, it's helpful to move any calls to func_get_arg() and func_get_args() to the start of the function (even when the original location was safe in practice), in order to reduce cognitive load for the reader.

Making this change can also eliminate warnings (sometimes false-positive) from certain code linters. e.g. "[Warning] Function argument(s) returned by "func_get_args" might have been modified."

Refer to https://secure.php.net/manual/en/migration70.incompatible.php#migration7...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jweowu created an issue. See original summary.

jweowu’s picture

Related issues: +#3006201: Errors with PHP7
jweowu’s picture

Status: Active » Needs review
FileSize
3.34 KB

Most calls to func_get_args() were already happening early. This patch addresses the remainder.

DamienMcKenna’s picture

The change you made in views_revert_views() moves func_get_args() to later in the function?

jweowu’s picture

The diff does make it look slightly odd, but that moves $viewnames = _convert_csv_to_array(func_get_args()); to the top of the function.

 function views_revert_views() {
-  $views = views_get_all_views();
-  $i = 0;
   // The provided views names specified in the command.
   $viewnames = _convert_csv_to_array(func_get_args());
+  $views = views_get_all_views();
+  $i = 0;
jweowu’s picture

In the context of the reason behind this issue, it would also be entirely reasonable to change this to:

  $args = func_get_args();
  // The provided views names specified in the command.
  $viewnames = _convert_csv_to_array($args);

or, closer to the original code:

  $args = func_get_args();
  $views = views_get_all_views();
  $i = 0;
  // The provided views names specified in the command.
  $viewnames = _convert_csv_to_array($args);

I'm happy to re-roll.

MustangGB’s picture

For future clarity might be best to be consistent and go with a $args = func_get_args(); version.
That said, I think DamienMcKenna just misread the patch file.

jweowu’s picture

MustangGB’s picture

Status: Needs review » Reviewed & tested by the community
Parent issue: » #2960871: Plan for Views 7.x-3.23 release

Seems pretty straight forward.

DamienMcKenna’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thanks!

  • DamienMcKenna committed 5c44b7f on 7.x-3.x authored by jweowu
    Issue #3016530 by jweowu, MustangGB: Relocate calls to func_get_args()...
DamienMcKenna’s picture

Status: Reviewed & tested by the community » Fixed
jweowu’s picture

Damien, could you re-review the change I made to views_ui.module ?

Re-reading the patch, I'm aware that the way I've reordered things means that drupal_build_form($form_id, $form_state); is now called without the 'no_redirect' and 'rebuild_info' values which were previously in $form_state when this happened.

I obviously thought this was ok at the time, and now I'm not so sure. drupal_build_form() will trigger drupal_rebuild_form() if isset($_SESSION['batch_form_state']), so the sequence change no longer looks safe to me.

DamienMcKenna’s picture

Status: Fixed » Needs work

Good catch, yes, best to put those back.

jweowu’s picture

Status: Needs work » Needs review
FileSize
986 bytes
721 bytes

Here's the follow-up patch.

5c44b7f1 was the commit which added the preceding patch.

The new patch restores views_ui.module to its previous state (5c44b7f1^) except with (only) the code establishing $args moved.

diff-views_ui-5c44b7f1^-15.txt shows the difference between the previous state of views_ui.module and this new patch. If you commit this patch and check git diff 5c44b7f1^ views_ui.module you ought to see the same.

DamienMcKenna’s picture

Status: Needs review » Fixed

Awesome, thank you!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.