WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 5 months ago

#44363 closed defect (bug) (fixed)

Coding Standards: Assignments must be the first block of code on a line

Reported by: subrataemfluence Owned by: pento
Milestone: 5.1 Priority: normal
Severity: normal Version: 5.1
Component: Plugins Keywords: has-patch needs-testing
Focuses: coding-standards Cc:

Description

In wp-admin/admin.php (line 177) the $page_hook was not assigned as the first block.

Rather it is first directly checked against the return value of the function get_plugin_page_hook and the value is not null the function has been called again to assign the value to $page_hook variable.

Currently we have:

<?php
if ( ! $page_hook = get_plugin_page_hook( $plugin_page, $the_parent ) ) {
   $page_hook = get_plugin_page_hook( $plugin_page, $plugin_page );
   ...
}

Which could be avoided easily if we restructure the code like this:

<?php
$page_hook = get_plugin_page_hook( $plugin_page, $the_parent );
if ( null !== $page_hook ) {
 ...
}

Proposed patch added.

Attachments (3)

44363.diff (713 bytes) - added by subrataemfluence 13 months ago.
44363-2.patch (889 bytes) - added by jrf 12 months ago.
Updated patch - essential line had been removed.
44363-3.patch (1.4 KB) - added by subrataemfluence 12 months ago.

Download all attachments as: .zip

Change History (9)

#1 @subrataemfluence
13 months ago

  • Keywords has-patch added; needs-patch removed

#2 @netweb
12 months ago

  • Milestone changed from Awaiting Review to 5.0

@jrf
12 months ago

Updated patch - essential line had been removed.

#3 @jrf
12 months ago

@subrataemfluence @netweb Looks like the original patch removed an essential line of code. I've uploaded a renewed patch which fixes that.

Props @johnbillion for spotting it.

Last edited 12 months ago by jrf (previous) (diff)

#4 @subrataemfluence
12 months ago

  • Keywords needs-testing added

Thank you @johnbillion and @jrf for spotting and correcting the patch. However, I think the new patch has a logical issue.

<?php
$page_hook = get_plugin_page_hook( $plugin_page, $the_parent );
if ( null !== $page_hook ) {
   $page_hook = get_plugin_page_hook( $plugin_page, $plugin_page );
   ...
}

The above will generate error.

We should only retrieve the plugin page hook sending current $plugin_page in both parameters if null is returned using $the_parent. Can this be the correct approach?

<?php
$page_hook = get_plugin_page_hook( $plugin_page, $the_parent );
if ( null === $page_hook ) {
   $page_hook = get_plugin_page_hook( $plugin_page, $plugin_page );
}
...

The entire code block looks like:

<?php
if ( isset( $plugin_page ) ) {
   if ( ! empty( $typenow ) ) {
      $the_parent = $pagenow . '?post_type=' . $typenow;
   } else {
      $the_parent = $pagenow;
   }

   $page_hook = get_plugin_page_hook( $plugin_page, $the_parent );
   if ( null === $page_hook ) {
      $page_hook = get_plugin_page_hook($plugin_page, $plugin_page);
   }

   // Back-compat for plugins using add_management_page().
   if ( empty( $page_hook ) && 'edit.php' == $pagenow && '' != get_plugin_page_hook( $plugin_page, 'tools.php' ) ) {
      // There could be plugin specific params on the URL, so we need the whole query string
      if ( ! empty( $_SERVER['QUERY_STRING'] ) ) {
         $query_string = $_SERVER['QUERY_STRING'];
      } else {
         $query_string = 'page=' . $plugin_page;
      }
      wp_redirect( admin_url( 'tools.php?' . $query_string ) );
      exit;
   }

   unset( $the_parent );
}

#5 @pento
8 months ago

  • Milestone changed from 5.0 to 5.1

#6 @pento
5 months ago

  • Owner set to pento
  • Resolution set to fixed
  • Status changed from new to closed

In 44598:

Coding Standards: Move an assignment out of a condition in wp-admin/admin.php.

Props subrataemfluence, jrf, pento.
Fixes #44363.

Note: See TracTickets for help on using tickets.