News Module: Missing Delete Icon for Field Definitions Tab

[neis]

I just installed/upgraded to CMS 1.2.2 and its News module version 2.6.1 to take advantage of the new "Field Definitions" feature. To understand how it works I added my first field to play with and than I started adding few more. I soon realized that I will not need all the fields I just defined so I wanted to delete some but ... there was no "Delete" icon available! After checking the HTML source of the admin page, I noticed the space for it but no link.

Checking deeper into the News module code, I found some potential errors in "function.admin_customfieldstab.php".

The code:

Code: Select all

    if( !in_array($row['id'],$usedfields) )
      {
	$onerow->deletelink = $this->CreateLink($id, 'admin_deletefielddef', $returnid, $gCms->variables['admintheme']->DisplayImage('icons/system/delete.gif', $this->Lang('delete'),'','','systemicon'), array('fdid'=>$row['id']), $this->Lang('areyousure'));
      }

says that if the current row id IS NOT in the list than create and the "delete" icon code.

My opinion is that the negation "!" is an error.

After I removed the above negation, another error appeared. The following code from the beginning of the file:

Code: Select all

$tmp = $db->GetArray('SELECT DISTINCT fielddef_id FROM '.cms_db_prefix().'module_news_fieldvals');
$usedfields = array();
foreach( $tmp as $row )
{
  $usedfields[] = $row['fielddef_id'];
}

should use the "module_news_fielddefs" and NOT "module_news_fieldvals" since one can have more fields defined than fields with data used in articles.

I changed the code like this:

Code: Select all

$tmp = $db->GetArray('SELECT DISTINCT id FROM '.cms_db_prefix().'module_news_fielddefs');
$usedfields = array();
foreach( $tmp as $row )
{
  $usedfields[] = $row['id'];
}

Now I can delete any of my Field Definitions.

[calguy1000]

Before News 2.6.1 was release significant testing was performed on the extra fields.  You cannot delete an extra field IF any of the news records have values for that fields.....

you didn't look quite well enough in the source:

Code: Select all

$tmp = $db->GetArray('SELECT DISTINCT fielddef_id FROM '.cms_db_prefix().'module_news_fieldvals');
$usedfields = array();
foreach( $tmp as $row )
{
  $usedfields[] = $row['fielddef_id'];
}

This code sets the $usedfields array.

[neis]

OK, I see your point though I am not convinced yet.

For example ... At this time I have just one Field Definition and only two active news articles. They existed before I did the upgrade. Till now I "played" only with one of them and I ended up not entering any data in the field, just clicking on the Submit button. After your post I went to my database with phpMyAdmin and verified that the table module_news_fieldvals has only one row (though the "value" field is blank). Than I clicked on my second news article to display it for editing and (without clicking anywhere else) I clicked the Submit button. Went back to my database and sure enough it shows now two rows, both with empty values for the "value" field.

Since I "now" I did not enter any value for the Field Definition in the edit news screen, I should be able to delete the Field Definition.

[calguy1000]

okay, so there may need to be some more tests in the article submission before rows are added to the fieldvals table, but the query I posted is correct

can you come up with a patch for the problem of submitting empty field values.... If so, I'll take a look and if it's valid, and works, I'll release a new News module.

and.... are there any other minor 'bugs'  (note, I said bugs not feature requests) that anybody has for News 2.6.1  (I hope you can reproduce them, because if you can't, I can't.).

News is a core module.... and one I'm particularly interested in at this time so if there are any important issues that can be found, I'll endeavor to fix them and release a new news module.

BTW:  if you report a bug, I expect you to help with the testing to ensure it's fixed.

neis: my post wasn't just to you   I appreciate your effort.

[calguy1000]

infact, after a cursory (and I do mean cursory) check of the addarticle stuff I see this code snippet:

Code: Select all

foreach( $params['customfield'] as $fldid => $value )
   {
        if( $value == '' ) continue;
        $query = "INSERT INTO ".cms_db_prefix()."module_news_fieldvals   
                (news_id,fielddef_id,value,create_date,modified_date) VALUES (?,?,?,?,?)";
        $dbr = $db->Execute($query,
                                            array($articleid,
                                                      $fldid,
                                                      $value,
                                                      $now,
                                                      $now));

Which tells me, that atleast an attempt is made to ensure that no 'invalid' data gets into the fieldvals table.... that means that there's probably some weird whitespace issue someplace.  I admit, that some better checking could be done, but this little check should ensure that if you didn't type anything, nothing gets inserted.