It would be useful to be able to adopt tables in the database that aren't managed by another module (that is, described via the SchemaAPI). Here is a patch to enable table adoption through data_ui; it depends on the Schema module.
I am not sure if there is some way I should be using the DataTable class, but it seems like all the methods assume you're either creating a fresh table, or that you're working with a table that Data already knows about.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 616324-5_data_adopt.patch | 6.33 KB | alex_b |
| #4 | data_adopt.patch | 6.27 KB | becw |
| #3 | 616324-3_adopt.patch | 6.71 KB | alex_b |
| #2 | data_adopt.patch | 5.61 KB | becw |
| data_adopt.patch | 2.94 KB | becw |
Comments
Comment #1
alex_b commentedThis is awesome.
Couple of things:
- I love the verbiage.
- Move the adopt logic from data_ui_adopt_form_submit() into DataTable::adopt($name, $table_schema).
- Add a help text to the adopt page and add an empty text for the case when there are no orphaned tables detected http://skitch.com/alexbarth/nr9fk/data-tables-localhost
- data_ui_adopt_form_submit() needs a comment.
Comment #2
becw commentedThanks for the feedback :) I added an 'adopt' method to the DataTable class, help text, and fresh comments.
Comment #3
alex_b commentedYou ran headlong into an issue I wanted to address earlier. The fact that putting clearing caches into CRUD functions isn't efficient if these functions are being batched.
The way how I'd like to address this is having no cache clear / rebuild functionality in CRUD methods at all and make it the API user's responsibility to clear caches after operations. I added DataTable::cacheClear() and modified the adopt patch accordingly as a first step in the right direction.
Other changes:
- Passing in the table name into adopt is redundant
- Minor tweaks to naming conventions to keep consistent with rest of code (i. e. call table $table, not $tmp. Nitty gritty stuff)
- unset() table pointer after adopting the table.
- Make $schema an optional parameter for adopt() so that API users can rely on DataTable to inspect the table. This keeps adopt() more in the pattern of create().
Needs review.
Comment #4
becw commentedooh, this looks good! I was wondering about a clearCaches() method because as you note I did run headlong into it. Two questions:
(1) Given that
DataTable::adopt()assumes we will always be adopting the$this->nametable, why take the$table_schemaas an argument at all? Is passing in the table schema redundant as well? I'm attaching a version of the patch that doesn't use the$table_schemaargument...(2) With
DataTable::clearCaches()now available, should cache clearing code be removed fromDataTable::create()andDataTable::addField(), and thenclearCaches()be called by eitherdata_create_table()or the form submit functions in data_ui? I suspect that this might influence other code out there, and that perhaps it can and should be addressed after this, in another patch/issue.Comment #5
alex_b commented1) Yes, good point.
2) Right, this would be a separate issue, just created one #618064: Remove cache clearing code from DataTable CRUD methods
Minor cleanup: adopt() does not need a table name as parameter any more. Removed it from call.
This is RTBC I'd say.
Comment #6
alex_b commentedCommitted to DRUPAL-6--1. Thanks for the contribution, this is a great feature.
Comment #7
becw commentedYay, thank you!