WordPress.org

Make WordPress Core

Opened 11 days ago

Last modified 7 days ago

#44363 new defect (bug)

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

Reported by: subrataemfluence Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version: trunk
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 11 days ago.
44363-2.patch (889 bytes) - added by jrf 9 days ago.
Updated patch - essential line had been removed.
44363-3.patch (1.4 KB) - added by subrataemfluence 7 days ago.

Download all attachments as: .zip

Change History (7)

#1 @subrataemfluence
11 days ago

  • Keywords has-patch added; needs-patch removed

#2 @netweb
10 days ago

  • Milestone changed from Awaiting Review to 5.0

@jrf
9 days ago

Updated patch - essential line had been removed.

#3 @jrf
9 days 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 9 days ago by jrf (previous) (diff)

#4 @subrataemfluence
7 days 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 );
}
Note: See TracTickets for help on using tickets.