Make WordPress Core

Opened 18 months ago

Closed 12 months ago

Last modified 12 months ago

#56982 closed defect (bug) (fixed)

Empty ELSE statement in dbDelta()

Reported by: krunal265's profile krunal265 Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.3 Priority: normal
Severity: trivial Version: 1.5
Component: Upgrade/Install Keywords: dev-feedback
Focuses: coding-standards Cc:

Description

In the wp-admin/includes/upgrade.php there is an empty else statement.
Line: 2740

Change History (7)

#1 @hellofromTonya
18 months ago

  • Keywords changes-requested removed
  • Version changed from 6.1 to 1.5

This empty else has been in the source code at least since WP 1.5. Resetting the Version to 1.5 though could have been early.

#2 @SergeyBiryukov
18 months ago

Hi there, welcome to WordPress Trac! Thanks for the ticket.

Just to save some time for anyone looking at the ticket, this is what the code in question looks like:

// Create a tablename index for an array ($cqueries) of queries.
foreach ( $queries as $qry ) {
	if ( preg_match( '|CREATE TABLE ([^ ]*)|', $qry, $matches ) ) {
		$cqueries[ trim( $matches[1], '`' ) ] = $qry;
		$for_update[ $matches[1] ]            = 'Created table ' . $matches[1];
	} elseif ( preg_match( '|CREATE DATABASE ([^ ]*)|', $qry, $matches ) ) {
		...
	} else {
		// Unrecognized query type.
	}
}

Introduced in [1575]. I believe the purpose of the empty else statement is to provide a comment to clarify the case when the query type is not recognized. It adds some context, though does not seem to affect anything further in the function. Does anyone have a strong opinion on whether it should be kept or removed?

#3 @brookedot
18 months ago

Does anyone have a strong opinion on whether it should be kept or removed?

My two cents either add an error/log message with something like wp_error or remove it. I don't find the comment particularly helpful and we could easily rework the code comments to not require the empty else

#4 follow-up: @costdev
16 months ago

  • Keywords dev-feedback added
  • Severity changed from major to trivial

I don't have strong feelings, although I do wonder whether the comment adds anything beneficial to the reader's understanding of the code.

We could change the initial comment:

Create a tablename index for an array ($cqueries) of queries.

to

Create a tablename index for an array ($cqueries) of recognised query types.

In addition, if we're going to remove the comment and else, we could also use continue to help separate each case and help a little with readability by not having a wall of if/elseif.


Current

<?php
// Create a tablename index for an array ($cqueries) of queries.
foreach ( $queries as $qry ) {
        if ( preg_match( '|CREATE TABLE ([^ ]*)|', $qry, $matches ) ) {
                $cqueries[ trim( $matches[1], '`' ) ] = $qry;
                $for_update[ $matches[1] ]            = 'Created table ' . $matches[1];
        } elseif ( preg_match( '|CREATE DATABASE ([^ ]*)|', $qry, $matches ) ) {
                array_unshift( $cqueries, $qry );
        } elseif ( preg_match( '|INSERT INTO ([^ ]*)|', $qry, $matches ) ) {
                $iqueries[] = $qry;
        } elseif ( preg_match( '|UPDATE ([^ ]*)|', $qry, $matches ) ) {
                $iqueries[] = $qry;
        } else {
                // Unrecognized query type.
        }
}

Proposed

<?php
// Create a tablename index for an array ($cqueries) of recognised query types.
foreach ( $queries as $qry ) {
        if ( preg_match( '|CREATE TABLE ([^ ]*)|', $qry, $matches ) ) {
                $cqueries[ trim( $matches[1], '`' ) ] = $qry;
                $for_update[ $matches[1] ]            = 'Created table ' . $matches[1];
                continue;
        }

        if ( preg_match( '|CREATE DATABASE ([^ ]*)|', $qry, $matches ) ) {
                array_unshift( $cqueries, $qry );
                continue;
        }

        if ( preg_match( '|INSERT INTO ([^ ]*)|', $qry, $matches ) ) {
                $iqueries[] = $qry;
                continue;
        }

        if ( preg_match( '|UPDATE ([^ ]*)|', $qry, $matches ) ) {
                $iqueries[] = $qry;
        }
}

Thoughts?

#5 in reply to: ↑ 4 @SergeyBiryukov
12 months ago

  • Milestone changed from Awaiting Review to 6.3
  • Summary changed from Empty ELSE statement to Empty ELSE statement in dbDelta()

Replying to costdev:

In addition, if we're going to remove the comment and else, we could also use continue to help separate each case and help a little with readability by not having a wall of if/elseif.

Yeah, I agree that looks better. Thanks!

#6 @SergeyBiryukov
12 months ago

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

In 55688:

Coding Standards: Remove an empty else statement in dbDelta().

Use continue to help separate each case for better readability, instead of having a wall of if/elseif.

Includes simplifying a similar fragment in make_site_theme_from_default().

Follow-up to [1575], [2037], [2040], [2044], [2346], [7999], [14080], [14485].

Props costdev, krunal265, hellofromTonya, brookedot, SergeyBiryukov.
Fixes #56982.

#7 @SergeyBiryukov
12 months ago

In 55698:

Coding Standards: Break out of the inner loop in make_site_theme_from_default().

This more closely matches the previous behavior with multiple if/elseif statements.

Follow-up to [55688].

See #56982.

Note: See TracTickets for help on using tickets.