[Done v157a] Spurious error message Query incomplete: missing closing semicolon.

[brittainmark]

I believe that there are a couple of faults in the sqlpatch.php this is used to apply sql patches in admin.
The first is the production of the error message Query incomplete: missing closing semicolon when using multi line sql statements.

In order to process more complex sql patches where you refer to more than one table on the database or the same table multiple times it is necessary top split the statement over multiple lines as stated in the help on the page. You can see this by clicking the details button in the install sql patched page.

INSERT INTO tablename
(col1, col2, col3, col4)
SELECT col_a, col_b, col_3, col_4
FROM table2;

Above will produce 3 errors followed by a success statement.

The second fault is the #NEXT_X_ROWS_AS_ONE_COMMAND: has no effect.
If you turn on debug you can see that the value is overridden as soon as the next statement is processed.
Statement

#NEXT_X_ROWS_AS_ONE_COMMAND:2
SET @Default_Insurance = "20.00";
UPDATE configuration SET configuration_value = @Default_Insurance WHERE configuration_key = 'MODULE_SHIPPING_RM1STSMLPARCEL_MAX_ORDERVALUE' ;

debug output

#NEXT_X_ROWS_AS_ONE_COMMAND:2 SET @Default_Insurance = "20.00"; UPDATE configuration SET configuration_value = @Default_Insurance WHERE configuration_key = 'MODULE_SHIPPING_RM1STSMLPARCEL_MAX_ORDERVALUE' ;
#NEXT_X_ROWS_AS_ONE_COMMAND:2
SET @Default_Insurance = "20.00";

About to execute.
Debug info:
$ line=SET @Default_Insurance = "20.00";
$ complete_line=1
$ keep_together=1
SQL=SET @Default_Insurance = "20.00";

UPDATE configuration SET configuration_value = @Default_Insurance WHERE configuration_key = 'MODULE_SHIPPING_RM1STSMLPARCEL_MAX_ORDERVALUE' ;

About to execute.
Debug info:
$ line=UPDATE configuration SET configuration_value = @Default_Insurance WHERE configuration_key = 'MODULE_SHIPPING_RM1STSMLPARCEL_MAX_ORDERVALUE' ;
$ complete_line=1
$ keep_together=1
SQL=UPDATE configuration SET configuration_value = @Default_Insurance WHERE configuration_key = 'MODULE_SHIPPING_RM1STSMLPARCEL_MAX_ORDERVALUE' ;

Keep_together is set to 1 for every transaction.

As it turns out it does not matter as it appears that the SQL statements are run with memory of the previous statements so as in their first example

#NEXT_X_ROWS_AS_ONE_COMMAND:4
SET @t1=0;
SELECT (@t1:=configuration_value) as t1
FROM configuration
WHERE configuration_key = 'KEY_NAME_HERE';
UPDATE product_type_layout SET configuration_value = @t1 WHERE configuration_key = 'KEY_NAME_TO_CHECK_HERE';
DELETE FROM configuration WHERE configuration_key = 'KEY_NAME_HERE';

If you leave out the #NEXT_X_ROWS_AS_ONE_COMMAND:4 the statement will still be process correctly


Proposed solutions:

1) Remove the check on missing semi colon and all reference to #NEXT_X_ROWS_AS_ONE_COMMAND: from the help page
Remove check from line 283;

        else {
            $messageStack->add(ERROR_LINE_INCOMPLETE, 'error');
        } 

OR

2) If you want to keep the missing semi colon check repurpose the #NEXT_X_ROWS_AS_ONE_COMMAND: to be the number of line that the single SQL statement is on.

This will require correcting the processing between lines 71 and 81
currently

    foreach ($lines as $line) {
    if ($_GET['debug'] == 'ON') {
      echo $line . '<br>';
    }

    $line = trim($line);
    $line = str_replace('`', '', $line); //remove backquotes
    $line = $saveline . $line;
    $keep_together = 1; // count of number of lines to treat as a single command 

new

   $keep_together = 1; // count of number of lines to treat as a single command
   foreach ($lines as $line) { 
     if ($_GET['debug'] == 'ON') {       
       echo $line . '<br>';     
     }
      $line = trim($line);
     $line = str_replace('`', '', $line); //remove backquotes
     $line = $saveline . $line;

Remove existing processing for #NEXT_X_ROWS_AS_ONE_COMMAND:
line 276

       $lines_to_keep_together_counter++;
        if ($lines_to_keep_together_counter == $keep_together) { // if all grouped rows have been loaded, go to execute.
          $complete_line = true;
          $lines_to_keep_together_counter = 0;
        } else {
          $complete_line = false;
        } 

replace with

          $complete_line = true;
          $lines_to_keep_together_counter = 0;  // to reset the counter if set incorrectly 

Add processing in before missing semicolon check line 283

else {
        $lines_to_keep_together_counter++;
        if ($lines_to_keep_together_counter == $keep_together) { // if all grouped rows have been loaded, go to execute.
          $lines_to_keep_together_counter = 0;
          $messageStack->add(ERROR_LINE_INCOMPLETE, 'error');
        } 
     } 

Additionally modify documentation in the sqlpatch.php language file accessed by pressing the Details button.



Finally I might suggest changing the button called "Details" to "Help"
admin>includes>languages>english.php line 282

 define('IMAGE_DETAILS', 'Details');

Might be better to add new button as this is used in orders and geo_zones as well



Hope that all make sense. There was more discussion on https://www.zen-cart.com/showthread....semicolon-quot

[mc12345678]

brittainmark and I had been talking about this in: https://www.zen-cart.com/showthread....semicolon-quot

I've looked over the above suggestion as well as the code and its operation in both the admin as well as the zc_install process.

To date it appears that #NEXT_X_ROWS_AS_ONE_COMMAND: has been used really for a visual indication to those reviewing the sql more than for the code supported by the logic that brittainmark identified where $keep_together gets reset to 1 effectively on the next line after the comment is processed.

The most recent issue about which this thread is mostly based is that there was a change: https://github.com/zencart/zencart/pull/3288
That was proposed because it seems that when parsing the sql file that if the very last line did not have a semi-colon, then the content of that line and preceding since the last semi-colon did not actually get processed.

If a statement in the middle of the sql did not have a semi-colon, then unless the combination of that line with the subsequent line provided a satisfactory sql statement, a sql error would be issued...

I propose that a "small" correction to this issue would be after the loop is complete to potentially evaluate the status of $newline and if its value when possibly using trim or removal of ending "spaces" by another process evaluate if $newline is empty or not. If it is empty, to *then* issue the message about the missing ending semi-colon...

That is unless, the purpose of adding the message is in some way to prevent providing more of an error check before attempting to present the sql to the processor and thus capture the issue in advance of attempting to execute the sql... It seems to me though that if that is the goal then the parser should perhaps be more robust in its checking.

I don't necessarily advocate the correction to the use of #NEXT_X_ROWS_AS_ONE_COMMAND: as that too adds a sort of level of potential problem that one not count correctly when implementing or changing that value. I note also that the zc_install process as found in: zc_install/includes/classes/class.zcDatabaseInstaller.php has the same "issue" where the collected number gets overwritten on the very next line of the sql file with no real processing occurring based on the value that is obtained.

Such that would change beginning at line 327:

  } // end foreach $lines  zen_record_admin_activity('Admin SQL Patch tool executed a query.', 'notice');
  return array('queries' => $results, 'string' => $string, 'output' => $return_output, 'ignored' => ($ignored_count), 'errors' => $errors);

to:

  } // end foreach $lines

  if (zen_not_null($newline)) {
    $messageStack->add(ERROR_LINE_INCOMPLETE, 'error'); // Why not attempt to process this line instead of alert about it?
  }
  zen_record_admin_activity('Admin SQL Patch tool executed a query.', 'notice');
  return array('queries' => $results, 'string' => $string, 'output' => $return_output, 'ignored' => ($ignored_count), 'errors' => $errors);

Then working backwards remove lines 284-286:

        else {
            $messageStack->add(ERROR_LINE_INCOMPLETE, 'error');
        }

[DrByte]

In looking back at the code history, as @mc12345678 mentioned, the change was made in:
https://github.com/zencart/zencart/p...f04e2b07fda7a7

The posted reason for making that change was that "uploaded SQL statements without ending semicolon were failing silently".

So, while we could back out the change as you propose, we're back to the original problem still.

Thoughts?

[brittainmark]

As I originally posted you could reuse the #NEXT_X_ROWS_AS_ONE_COMMAND: I know it is prone to error if you count wrong. I had also suggested that if a semicolon is encountered that is assumed to be the end of the command and the counter is reset (or an other error could be put in to say that the counter is wrong). This way you do not get silent fails. If people test their scripts then they will know if they are wrong and you do no get false failure messages because of multi line sql commands.

Another option is to change the error message to a meaningful warning message.

Or leave as is and do as i have in big royal mail document the number of expected error messages that occur when the script is run.

[brittainmark]

Any progress on this?

[DrByte]

v157a includes this fix: https://github.com/zencart/zencart/pull/3946/files