[BUG] 175 - updatefabrikDatabase() logic booboo

[cheesegrits]

Rob,

I think I've identified where people are having issues with the "job application" tutorial, and other new form / table weirdness in general.

In the tutorial, it tells you to create and save the form, then immediately go to the Tables page and "Update Database":

Create the Form:
[blah blah]
3. Fill in the form with the following information:
[blah]
4. Select "Record Form Results Database".
[blah]
6. Press "Save". This will return you to the list of "Currently Available Forms".

Create the Fabrik Table:
[blah]
Click the "Update Database" link at the extreme right of the "job types" row on the Fabrik form page. This will create a database table called "fabrik_formdata_x" where "x" is the form's id. You will then receive an "Update Successful" confirmation message.

Click on the "Create Table View" link to create a Fabrik table view of the database table.

The problem being, if you do this, Fabrik gets its panties in a wad because of the logic in the updatefabrikTable() function:

function updatefabrikDatabase( $formId ) {
	
	global $database, $mosConfig_dbprefix;
	$oForm = new fabrikForm( $database );
	$oForm->load( $formId );
	//use this in case there is not table view linked to the form 
	$databaseTableName = $mosConfig_dbprefix."fabrik_formdata_".$formId;
	$thisDb = $database;
	if ( $oForm->table_id > 0 ) {
		//there is a table view linked to the form so lets load it
		$oTable = new fabrikTable( $database );
		$oTable->load( $oForm->table_id );
		$oConn = $oTable->getConnection( );
		$databaseTableName = $oTable->db_table_name;
		$thisDb = $oTable->_oConnDB;
	}
	if ( !$oTable->databaseTableExists( ) ) {
		$oTable->createFormTable( $oForm );
	} else {
		$oTable->ammendTable( $oForm );
	}
}

As far as I can tell, the problem is because the table view hasn't been created yet, there is no $oForm->table_id ... so the $oTable object doesn't get created. So when it tests for !$oTable->databaseTableExists() ... weirdness ensues. I think the extent of the weirdness depends on how you have PHP set to handle errors.

I suspect the fix is to a) move that databaseTableExists test inside the "if ( $oForm->table_id > 0 )" block, and b) reword the tutorial appropriately, so it explains that when you save the Form (with "Record to database" selected) it will automagically create the table at that point, and the first thing you should then do is "Create Table View" (NOT "Update table").

-- hugh

 

[cheesegrits]

OK, I think the problem actually lies elsewhere, and after much head banging, I think I found it.

The problem is actually in _saveFormGroup(), in fabrik.class.php. If you are saving a form with "record in database" set for the first time, Fabrik creates the form for you. Which works great. BUT:

				if ( !$oTable->databaseTableExists( $oTable->db_table_name, $database ) ) {
					//need to pass the correct database obj here
					$oTable->createFormTable( $this );
				} else {
					$oTable->ammendTable( $this );
				} 
				$oTable->state = $this-> state;
				$oTable->label = $this->form_title;
				$oTable->form_id = $this->id;
				$oTable->connection_id = $oConn->id;
				$oTable->store();
				$oForm->table_id = $oTable->id;
				$this->store();

Note that second to last line. It's grabbing the table id from $oTable->id and putting it in $oForm->table_id. Only ... there is no $oForm. That line should be setting $this->table_id.

Upshot is, the form gets saved, BUT without the table_id.

So the next time Fabreik tries to update the table for whatever reason, things get REALLY funky, because although there is already a database, Fabrik tries to create another on.

I'm reasonably sure this is the problem. I'm testing the fix now.

-- hugh

 

[cheesegrits]

Yup, that was the little booger. With that fix, the form/table are now properly associated after the auto-creation, you no longer get that spurious "Create Table View" option on the Forms list, and "Update Database" no longer craps out. No more second table being created, etc.

I suspect this one has been the cause of a lot of table / form problems, and is definitely why people working through the Job Applications tutorial are having problems. That tutorial needs to be reowrded, to take account of the fact that the table is now created automagically, you don't need to go through the "Create Table" phase.

-- hugh

 

[rob]

ah right - nice catch ;D

do you want to commit the change?

Rob

 

[cheesegrits]

Well, it seems like there is another lurking problem. Although the table_id fix addresses one issue, there seems to be some more weirdness ... the test table I just created via a new form ended up with three sets of fabrik_internal_id and time_date columns!!

I'll delve into that stuff tomorrow, it's getting kinda late ... until then, I suggest we hold of committing any changes.

-- hugh

 

[cheesegrits]

Rob - I could probably use an assist here. This is obviously a serious bug, and the code is fairly non-trivial, so it's taking me a while to work out what is supposed to be happening, versus what is actually happening.

I've gotten as far as confirming this behavior. It doesn't actually create multiple columns in the table, rather it creates multiple 'fabrik_internal_id' and 'time_date' elements. Neither of the duplicated id's are set as primary or autoincrement. So after creating my new test form, which has a single text Element in a single Group, I have:

mysql> select id, element_name, group_id, elementtype_id, element_label from jos_fabrik_elements where group_id = 8;
+----+--------------------+----------+----------------+---------------+
| id | element_name    | group_id | elementtype_id | element_label |
+----+--------------------+----------+----------------+---------------+
| 76 | yet_another_test  |    8 |       2 | Test Again  |
| 77 | fabrik_internal_id |    8 |       2 | id      |
| 78 | time_date     |    8 |       2 | time_date   |
| 79 | fabrik_internal_id |    8 |       2 | id      |
| 80 | time_date     |    8 |       2 | time_date   |
+----+--------------------+----------+----------------+---------------+
5 rows in set (0.00 sec)

More News When There Is News,

-- hugh

 

[cheesegrits]

Jeez, this one is a doozy.

I can no longer replicate the 'multiple elements' behavior. It is conceivable that may have been down to orphaned data from previous tests causing a "Panties In A Wad" condition, I'm not sure. I was trying to re-use the elements/group in each test, rather than scraping everything clean between each test case and starting everything from scratch.

HOWEVER ... I can replicate another bug, which is:

After the table is auto-created, the first time you edit the table, it bitches about the primary key not being set. Which is bogus, because the auto-create process created fabrik_internal_id, and set it to be the primary key, auto incrementing. I know, 'cos I've watched it do it like a zillion times in the last 24 hours, LOL!

But the first time you edit the new table in the Tables admin page, after the $row->load($id) in editTable(), $row->db_primary_key is not set.

So I guess somewhere in the auto-create, even though its actually creating the fabrik_internal_id and correctly setting it as primary and autoinc, it must be spacing on actually setting the db_primary_key and auto_inc values in the jos_fabrik_tables row.

Still, I actually feel like I'm making some progress ... light at the end of the tunnel and all that!

-- hugh

 

[cheesegrits]

OK, I think that was it. In CreateFormTable(), although it was correctly creating the default fabrik_internal_id Element, and creating it as the primary key in the new table itself, it wasn't doing this:

		$this->db_primary_key = 'fabrik_internal_id';
		$this->auto_inc = 1;

So the row written out to jos_fabrik_tables for the new table didn't have those two properties set. So as far as Fabrik itself is concerned, the table has no primary key. Double Plus Ungood.

Between this fix, and the table_id thing, I think auto-creation is now sane again. I'll commit these as my first changes direct to SVN.

And now I've got SVN commit access (thank you, Rob), I'll probably move these BUG threads (where I basically talk to myself for a day or so!) to the 1.0 Development forum, to help keep the noise down in the Support forum.

-- hugh

 

[rob]

hi Hugh

Sounds good!

Cheers
Rob