Using the site-wide contact forms, any subjects with an '&' cannot be deleted. Clicking delete for these subjects brings up the expected action flow, however, confirming the delete does nothing. Subject still exists.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

m3avrck’s picture

It appears this issue might be linked with this one: http://drupal.org/node/23685

A problem with mod_rewrite and not the actual contact module.

Souvent22’s picture

FileSize
4.7 KB

Redid how contacts work. It now references contacts by ID instead of their category as the unique identifier. It also allows for contacts to have weights, so that one may control where the contact will appear in the listing.

Souvent22’s picture

FileSize
27.23 KB

This is the update.inc for the database cahnges that goes along with this patch.

Souvent22’s picture

FileSize
4.5 KB

Updated patch.

Souvent22’s picture

FileSize
1.29 KB

Updated database patch.

Cvbge’s picture

Status: Active » Needs work

Hi,

0. You should keep all changes in one file, i.e. pake one patch, not patch for every file
1. You should include changes for database/database.{mysql,pgsql}
2. IIRC auto_increment was depreciated, you should use db_next_id() (but I see auto_increment in cvs...)
3. You use "WHERE cid = '%s'", but cid is integer so this should probably be %d (although this probably would not break anything..)

Souvent22’s picture

Status: Needs work » Needs review
FileSize
6.38 KB

Ok, I've taken your suggestions in, and i think I have it this time. Let me know if this is a correct format for a patch. This only fixes the bug of not being able to delete special characters.

m3avrck’s picture

Check your database updates, the UNIQUE fields don't look right syntactically.

Souvent22’s picture

FileSize
6.25 KB

Ok, i apologize for the last upload. Pleasle ignore. I send the wrong file. :-[. Anyway, here's a tested, tried, and correct patch.

m3avrck’s picture

+1 patch works great! Latest HEAD version applied with update.php works great, modifies database correctly for MySQL (and PostgreSQL looks correct as well) and contacts can now be deleted if they have '&' in the subject. Also, dumped entire database and created from modified database.mysql and everything works great. I'd say this patch is ready to be committed!

Cvbge’s picture

Hi, patch does not applies to currect cvs anymore.

For the database.pgsql you need to remove (11) from int(11) and also first "category" from UNIQUE line, so it looks like this:

CREATE TABLE contact (
  cid int NOT NULL,
  category varchar(255) NOT NULL default '',
  recipients text NOT NULL default '',
  reply text NOT NULL default '',
  UNIQUE (category),
  PRIMARY KEY (cid)
);

The updates are also not compatibile with postgres, but postgres upgrade path is completly broken in cvs and I'll fix it later, so do not pay attention to it.

Souvent22’s picture

Hello. Thanks for the feedback. I'm in the middle of fixing it, but i have a question. You'll have to excuse my ignorance with PostGres, I'm not as familiar with it as I am MySQL. With that said, I have a question.....
I am trying to make my update where one does not lose data. However, in PostGres, I have to make a seperate "temp" table before hand and then copy the date over the the new table (cause i don't know the exact name of the constraint to drop e.g. the primary key).

Basically I'm asking if there is a better suggestion in which I doint have to use a temp table, and i could just manipulate the original table? Thanks.

Souvent22’s picture

Also,

I created a temp name for my temp table. However, for prefix purposes, should I enclose it? Example:

$temp_name = rand();
$res = db_query("SELECT * FROM $temp_name");

or

$temp_name = rand();
$res = db_query("SELECT * FROM {$temp_name}");

Thanks.

Souvent22’s picture

Clarification:

$temp_name = rand();
db_query("CREATE TABLE $temp_name AS SELECT * FROM {contact}");
$res = db_query("SELECT * FROM $temp_name");

or

$temp_name = rand();
db_query("CREATE TABLE {$temp_name} AS SELECT * FROM {contact}");
$res = db_query("SELECT * FROM {$temp_name}");
Cvbge’s picture

cause i don't know the exact name of the constraint to drop e.g. the primary key

Yes, you know it, it's tablename_pkey. In this case it's contact_pkey. You can drop it by dropping index with it's name (you could drop constraint with it's name but it was not available in 7.2)
Adding a column and adding unique attribute is also doable without temp table. You can look at (some) previous updates. But some of them are incorrect ;)

By saying that I'll fix it later I meant that I want to introduce function to modify tables and don't want to make temporary fixes now.

Souvent22’s picture

Assigned: Unassigned » Souvent22

Thanks. yes, you're right. didn't know if that was universal across the board though since that'st he default, but someone could give their primary key constraint a diff. name. But, with your comment, I'll wait and let you take care of fixing my pgsql portion of my patch. Thanks for your comments though, really helpful...and not confusing. :).

Souvent22’s picture

FileSize
6.23 KB

Got the postgres correct this time. No temp table needed.

Souvent22’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
6.35 KB

patch re-roll due to head change.

chx’s picture

Does this merit a DB change? what about passing md5(subject) instead? or is that too hackish? seems to be lot simpler to me...

m3avrck’s picture

chx, md5() would work but that does seem a bit hackish ;)

Cvbge’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
6.81 KB

It's case 'pgsql' not case "pg_sql"

Also, I've noticed now that you use db_next_id().
First, the format is db_next_id('{tablename}_fieldname'), so in this case it should be db_next_id('{contact}_cid')
Second, using db_next_id() with postgres needs a sequence defined.

Also update path needs changes in case there are already some contacs in the table - the new column cid needs to be updated and sequences set.

I've fixed this [for postgres] and attached new patch. I'm not sure how mysql behaves when adding a primary key and there is an existing data, buy you probably have to take care of it (if mysql allows you to add a column that is a primary key and allows it to have nulls then... well, I don't want to swear ;))

I've tested the code and it seems to run ok.

Cvbge’s picture

Status: Needs work » Reviewed & tested by the community

About the update path: since this module is in cvs only, not in 4.6, then we probably should not worry about data being already in the table.
(Although drupal.org is already running on cvs with contact.module :))

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Patch looks good but needs a bit of work:

1. You don't need to urlencode numeric IDs: urlencode($category->cid)).
2. You can write %d rather than '%d' in queries.

Except for that (and without any testing), the code looks good. This change can go in after the code freeze; it's a bugfix so you might want to focus on other stuff first.

Souvent22’s picture

Status: Needs work » Needs review
FileSize
5.62 KB

Patch re-rolled against current HEAD.

Souvent22’s picture

Status: Needs review » Needs work
FileSize
5.59 KB

Re-roll.

Dries’s picture

Status: Needs work » Fixed

Committed to HEAD. Thanks.

Cvbge’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
6.88 KB

Fixes for the postgres part.
I had to change the db_{add,change}_column to be able to add default values which are not simply text values, but it's trivial change ;)

Cvbge’s picture

Component: contact.module » database system
Priority: Normal » Critical

My patch should also fix general update error reported in http://drupal.org/node/36571

Dries’s picture

Committed to HEAD. Thanks.

Cvbge’s picture

Status: Reviewed & tested by the community » Fixed
Anonymous’s picture

Status: Fixed » Closed (fixed)