I've seen this now so often... don't do this. I do not see the reason for this @ - and using it is bad practice and shouldn't be done for the reason that real errors are lost. There is no reason to use this here. We can use db_table_exists() to verify if table {metatags} exists or not. Otherwise I do not see any reason why this query should "fail".

/**
 * Implementation of hook_uninstall().
 */
function nodewords_basic_uninstall() {
  $table = db_table_exists('nodewords') ? '{nodewords}' : '{metatags}';

  $metatags = array(
    'abstract',
    'canonical',
    'copyright',
    'description',
    'keywords',
    'revisit-after',
    'robots'
  );

  @db_query("DELETE FROM $table WHERE name IN (" . db_placeholders($metatags, 'varchar') . ")", $metatags);

  db_query("DELETE FROM {variable} WHERE name LIKE 'basic\_metatags\_%'");
}

Comments

hass’s picture

hass commented

$table = db_table_exists('nodewords') ? '{nodewords}' : '{metatags}'; is another thing that seems useless if the upgrade path would work, well.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: Never supress errors with @. » Code is suppressing SQL errors in uninstallation phase
Status: Active » Fixed
hass’s picture

hass commented
Title: Code is suppressing SQL errors in uninstallation phase » Code is suppressing errors in many places of the install files
Status: Fixed » Needs work

I've referenced only one place above, but the .install files have more.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: Code is suppressing errors in many places of the install files » Code is suppressing errors in many places of install files 
    if (@unserialize(base64_decode($metatag->content)) !== FALSE) {
      continue;
    }

    if (($content = @unserialize(base64_decode($metatag->content))) === FALSE) {
      continue;
    }

Suppressing a warning message in that case is legitimate, as the code is checking the return value, and sees if an error happened; in this case, if there is an error, it means the value was not serialized.

hass’s picture

hass commented

If something need to be suppressed it shows more or less that there are unhandled exceptions. It's something that is used for sloppy coding if the developer(s) may have no idea what could happen and why something may fail without trying to understand. This may sounds very general, but it's how it's mostly used.

Another example. If the update hooks are working reliable there is no need to check for both tables. Updates need to run and the module must know what the name is. This is so confusing, why the module renames this tables many times back and forth. A clean-up would be really helpful.

  $table = db_table_exists('nodewords') ? '{nodewords}' : '{metatags}';
  $result = @update_sql("DELETE FROM $table WHERE type = 'robots'");

This are a few lines more, but they do not suppress something and they are much more readable (aside this buggy hook deletes user data -> data loss):

if (db_table_exists('nodewords')) {
  $ret[] = update_sql("DELETE FROM {nodewords} WHERE type = 'robots'");
}
elseif (db_table_exists('metatags'))
  $ret[] = update_sql("DELETE FROM {metatags} WHERE type = 'robots'");
}
else {
  // critical error occurred - module tables are missing - how is this possible??? - may force update hook to crash!
}

We have also seen in #193383: set_time_limit: Centralize calls and prevent warnings and errors, #111 how this broke core 6.14 and many other modules. It's bad practice to suppress errors with @ and should be avoided.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

It's something that is used for sloppy coding if the developer(s) may have no idea what could happen and why something may fail without trying to understand.

See my previous comment; I perfectly know when unserialize() returns FALSE, and I perfectly know what it means.
As the function returns an error code, and the function checks that error code, the function itself is perfectly working.
Let me turn the question: why would unserialize() return FALSE? What does that mean?

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Fixed

Whenever possible, the code has been already changed, and committed in CVS.

Status: Fixed » Closed (fixed)

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