FieldCrudTestCase::testUpdateField() does not test changing the schema

This is a simple patch, and fixes the error on sqlite.

The purpose of testUpdateField() is not to make sure that numeric columns work but that the field_update_field() function works. The current code assumes that creating a numeric(5,2) field works as expected, then changes it to numeric(5,3) to verify that the change takes. The proposed patch actually just disables the test on SQLite, because the assumption about numeric(5,2) fields is wrong, and the modified test actually does not verify that field_update_field() does anything (since the numbers are stored as floats, the test would pass if field_update_field() was not even called).

Really, the to be correct on dbs that have numeric columns, the test should first verify the field behaves as a numeric(5,2) should, and then verify that the updated field behaves as a numeric(5,3) column. But this issue is not about belt-and-suspendering this test case.

A correct patch would change the test to change a field in some way that verifies field_update_field() is actually working. Incidentally, if necessary I think it is completely acceptable to have branches in the test code based on database type. Tests are not normal code, they just need to do whatever is necessary to test what needs to be tested. So if a correct test on all db types requires if (driver() == 'sqlite'), so be it.

Alternatively, the critical issue can be fixed by disabling the test on sqlite altogether (again with an if statement), since really that is all the present patch does anyway, just in an obscured manner.

On SQLite, fixed-precision numeric(digit, precision) types do not exist, and everything is stored as either integers or floats.

From the Schema API:

'precision', 'scale': For type 'numeric' fields, indicates the precision (total number of significant digits) and scale (decimal digits right of the decimal point). Both values are mandatory. Ignored for other field types.

But sqlite completely ignores these parameters, so we need to change the comment there.
We no longer "support" fixed-precision fields (i.e. one can't rely on the fact that the values are rounded to a specific precision) , but perhaps we can support a kind of "minimal guaranteed precision" here?

Here's a simple patch that simply skips the test for sqlite.

reroll

I rewrote the test from ground up. It now changes cardinality from 4 to 5 and makes sure that amount of stuff can be inserted but not more.

dereine asked to check the values loaded. And I do not like the way the patch is structured. (ie. 4 is used twice)

This do while looks much better than before.

Looks fine.

looks good. +1 to RTBC.

This looks good, but that test is missing a big ass "why" you're using that method to test field updates, since it's kinda obscure. I wouldn't know from reading the issue, and only know because I happened to be in the room with chx when he explained it to me. :P

So I added this to the top of the first hunk:

    // Create a field with a defined cardinality, so that we can ensure it's
    // respected. Since cardinality enforcement is consistent across database
    // systems, it makes a good test case.

If there's a more betterer comment to describe this, feel free to post a follow-up patch to amend it.

Committed to HEAD, with that adjustment.

The original test, though flawed as described above, tested Field API altering the field storage schema. The new test does not; in fact the new test could pass without calling the field storage engine's update hook at all.

IMHO, we should have a field_update_field() test that changes the schema. Not critical, I suppose.

Barry, good luck figuring out a test that will test more than this and generic enough. I know that the mongodb field storage engine enforces from PHP this and will need an upgrade because currently it throws exceptions at invalid deltas. Edit: I mean, even this test needs PHP-level enforcement for mongodb and I have my doubts about enforcing anything else... Even in SQL, in SQLite you can store anything into any SQL field unless we begin to add CHECKs to the SQLite-schema code so I have no idea what else you can find...

Still edit:

  if ($has_data && $field['columns'] != $prior_field['columns']) {
    throw new FieldUpdateForbiddenException("field_sql_storage cannot change the schema for an existing field with data.");
  }

Didn't we agree that we put stuff into fields that are hard to upgrade, anyways?

In #3 I wrote: "Incidentally, if necessary I think it is completely acceptable to have branches in the test code based on database type. Tests are not normal code, they just need to do whatever is necessary to test what needs to be tested. So if a correct test on all db types requires if (driver() == 'sqlite'), so be it."

A more reasonable way to do that is to put the tests into the storage module's tests directly. Ironically, it turns out in this case we've ALMOST already done that. field_sql_storage.test has testUpdateFieldSchemaWithData() (the update should fail) and testFieldUpdateIndexesWithData() (the update should succeed). So, all we need to do is move the original 'numeric' test from field.test to field_sql_storage.test. Other storage engines which do not perform a similar test of field_update_field() will simply be less well tested than field_sql_storage.module.

Test moved to field_sql_storage.test and improved slightly.

Yeah, so, I forgot that the SQL storage engine runs on things other than mysql. :-) What can I say, I was up late last night.

I think the solution is to wrap the test in "if (driver() == 'mysql')" as described in #19.

Hey guys. I'm really new to all of this. I've worked a little on d6. I'm trying to make a website for my little cousins charity. I'm only 14, so this is all pretty overwhelming to me. I used to have a subscription on Lynda.com that helped me out a lot, but it expired. Anyway, I installed a suite of modules on my d7 installation to work for drupal commerce. I had to download the address field, entity API, rules, and views modules for it. When I enable all this modules, I get this error -

FieldException: Attempt to create field name billing_address which already exists, although it is inactive. in field_create_field() (line 277 of C:\Users\Daniel\Desktop\drupal-7.0-beta1\drupal-7.0-beta1\modules\field\field.crud.inc).

What can I do to fix this? download one of those files above? replace a file with one above? Thanks!
Dan

Resetting the title.

I would recommend you just reinstall your whole site from scratch. If any problem persists, please open a new issue in the Drupal core or Commerce issue queue. That way people will be more likely to help you than in an issue, which is focused on something entirely unrelated. Thanks.

Reroll with the db_driver() check. Only tested against SQLite currently; could someone with access to Postgres give it a run?