Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
db_index_exists accepts only the name of an index. What happens if there are two different tables with indexes of the same name? Is there a way to select the table? Should index names be unique? There is nothing in db_add_index to say indexes should have unique names and db_add_index accepts $table, $name.
Comment | File | Size | Author |
---|---|---|---|
#30 | 722912-index-exists-d7.patch | 10.79 KB | andypost |
#26 | 722912-index-exists-d7.patch | 12.7 KB | andypost |
#12 | 722912-index-exists-name-d7.patch | 6.38 KB | andypost |
#11 | 722912-index-exists-d7.patch | 5.79 KB | andypost |
#10 | 722912-index-exists-d7.patch | 4.79 KB | andypost |
Comments
Comment #1
peterx CreditAttribution: peterx commenteddb_index_exists accesses schema()->indexExists($name);
indexExists is:
D6 -> D7 conversion is failing in block because block deletes a non existent index. db_index_exists is needed to make block #7003 work. The function should be:
I will test with alpha2.
Comment #2
peterx CreditAttribution: peterx commentedTested in http://drupal.org/node/722920. The change works.
Comment #3
ctmattice1 CreditAttribution: ctmattice1 commentedNoticed this also when lookinG at the filter.update.
Besides index_exist was having problems with db_add_unique_key function.
could this possibly be related in some way to #720620: indexExists() for pgsql and sqlite does not prefix tablename with mysql silently failing
Comment #4
andypostThis is a good hit! Also related #718016: DatabaseSchema_pgsql::renameTable() needs to rename the indexes
The explanation could be: Mysql share index-name in namespace of the table but sqlite and pgsql index-name are global identifiers.
Also this is not critical
Comment #5
Crell CreditAttribution: Crell commentedHm. Irritating. Is there a way this can be handled without an API change, or do we just have to suck it up?
Comment #6
andypostOMG! db_add_index() and db_drop_index() uses table-name so this change is reasonable!!!
We could not proceed without changing API because:
- for mysql we should know real table-name to check index!
- for sqlite and pgsql we should prefix index with full table-name before check!
We we won't changing api:
- all db_index_exists should be changed to {table}_{index} format
- change mysql schema.inc to make index names prefixed by table-name like pgsql and sqlite now!
Anyway all core should be checked for using db_index_exists and contib also...
This is really critical!
Comment #7
chx CreditAttribution: chx commentedIf this is really critical can someone make a real summary which does not require us to dig out the signatures of the functions thrown around? "this is what we have (functions with signatures inlined). this does not work because (copypaste from above). The new signature should be because ...."
Comment #8
andypostThis is a required API change because
- signature of db_index_exists() is different from called schema->indexExists()
- we have no place in core that uses this function
Also all db-dependent implementations of indexExists() are different and does not conform abstract class
This is a first patch to make this change
Comment #9
andypost@Crell because this function is not used in core suppose you should decide to return value
Right now only sqlite works as described!!!
Also we already have tests for this but they are using schema api:
already in #720620: indexExists() for pgsql and sqlite does not prefix tablename
and waiting #718254: Fieldname in tests are too long - this could be converted to db_index_exists() to cover this api
Comment #10
andypostThis patch changes php-docs, result type for all db engines also includes modified test to strict check for result of API function!
Comment #11
andypostDocs extended for each database
Comment #12
andypostAnd this patch changes return value to Index-name as it was described in indexExists()
Tests also fixed.
Comment #13
chx CreditAttribution: chx commentedWhy do you need identical FALSE, why assertFalse is not enough? If we are working on this why not do security hardening? It seems we are throwing strings against the DB without any escaping. There is a table escape function already and it feels appropriate to use it for index as well I think.
Comment #14
andypost@chx I use assertIdentical() is required to check for TRUE and FALSE in #11 to be sure that result is boolean. So in #12 I just change a values. About security - suppose escaping should be done in calling module not this function because escaping could change the name, also other functions does not use escaping.
Anyway, #11 and #12 just a two possible ways to solve issue.
Comment #15
peterx CreditAttribution: peterx commentedThere is db_escape_table and that might also work on column names or field names. I personally think Drupal should protect against stuff passed in from outside. If all db functions implement the same escape for all table, column, and index names, people cannot create a column or field or index that cannot be tested by db_index_exists. Developers will be warned at creation time instead of later.
Plus db_escape_table is aimed at stopping the use of names that will not work in other databases. Again we should warn people when they go to create an index named
that it will not work with some databases and, even if the database accepts it, the file system might not.Perhaps one of the Drupal overlords, Webchick?, will document the line where we protect databases from developers who want to decorate identifiers with corinthian capitals.
Comment #16
Crell CreditAttribution: Crell commentedOne of the key goals for the DB layer is to provide a unified API for all databases so module developers can support all supported databases without realizing they're doing so; if that means we block anything but ASCII alphanumeric in table and index names just to be safe, so be it.
I'm fine with using $connection->escapeTable() (not the function wrapper) to filter index names.
@andypost: I believe $this->assertFalse() is more correct in this case.
I'll try to review the code tonight if I can. Sorry, about to leave town for a client meeting. :-(
Comment #17
Dries CreditAttribution: Dries commentedI'm asking Crell to sign-off on the final patch. I'm comfortable making API changes given that (i) it is a critical bug and (ii) the API doesn't seem to be used widely yet. Let's fix this properly.
Comment #18
andypostThere's a big difference in patches from #11 and #12 - first returns only booleans and second returns boolean and string index name!!!
So I thing that explicit type checking provided by assertIdentical() is required.
About using
$connection->escapeTable()
- suppose only Crell could decide on this because it require changes in all db-related functions...Comment #19
peterx CreditAttribution: peterx commentedI suggest the immediate need is to make db_index_exists work the same as db_column_exists, something that is already in use, because there are other issues waiting on the ability to specify the table name. If there is a need to change the way all db functions return values then it should be a separate issue.
Comment #20
Crell CreditAttribution: Crell commentedI talked this over with andypost in IRC and we reached the following conclusion:
1) There's columnExists(), tableExists(), and indexExists(). Right now they all return different things, and columnExists()'s return doesn't make the slightest bit of sense anyway. I suspect it was a copy/paste error from findTables().
2) We agreed that we should normalize all three operations, across all databases, to return the name of the thing being checked if it exists and boolean FALSE if not. Given that, I think assertIdentical() for the FALSE check is OK, since the variable being returned could be multi-type. That is, the approach in #12, although it needs to fix up columnExists() too while we're at it.
3) Upon further investigation, it looks like we're already being horribly inconsistent in escaping of table names. We escape them in all built queries, but not on table creation and not in static queries. Gah, how'd we miss that? :-) That's an entirely separate issue on its own, so I am going to say we should not deal with that here, do no additional escaping, and open a new issue to decide what to do with the table/index escaping.
4) I also noticed several places in schema where we're using db_query() instead of $connection->query(). I don't know how those snuck in. That's a separate issue as well. Just noting it here since it impacts the same code in places.
I think that covers everything.
Comment #21
andypostAlso inform that pgsql still have no columnExists() and tableExists() implementations - DamZ said that default works fine.
And another issue should be extended tests for db_* functions
Talked with DamZ on IRC - he thinks as me that TRUE/FALSE as result enough because:
- returning result as string name could be confusing - pgsql and sqlite are prefix index-names with table name so patch from #12 returns same $name as $name argument, but this seems useless because calling function already knows $name
- this require additional operations and memory for strings
Comment #22
chx CreditAttribution: chx commentedit's silly to return a string in a function called exists. noone expects that AND it's indeed a known string. if you'd look for an unknown string then it'd be find or search or something but not exists.
Comment #23
Crell CreditAttribution: Crell commentedI am not picky whether we normalize to name/false or true/false, as long as it does get normalized and documented as such.
Comment #24
andypost@Crell I see no docs about return value at api.d.o at all
http://api.drupal.org/api/function/db_column_exists/7
http://api.drupal.org/api/function/db_table_exists/7
http://api.drupal.org/api/function/db_index_exists/7 (this said TRUE/FALSE)
Drupal 6 versions also have no php-doc
EDIT: Different return values require different implementations and tests so I think we should decide on that first!
Comment #25
Crell CreditAttribution: Crell commentedI was looking at the code directly. The docblocks for it are all AFU. Those should get fixed as part of this issue.
Comment #26
andypostNew patch:
1) fixed all php-docs for db_*_exists() functions and their schema()->*Exists() analogs
2) return value unified to boolean TRUE/FALSE (I inspect all places in core - all of them use return value as boolean)
3) tests in schema.test are fixed to assertIdentical() for purpose that other possible db-drivers have ability to be tested for explicit return value of their schema implementations.
4) tests in database_test.test are grouped - they are about db_*_exists() functions same as 3)
5) As Crell pointed at #20-4) all db_query() (4) and one db_select() replaced with $connection->query() analogs
Comment #27
Crell CreditAttribution: Crell commentedAnd it's even extensively documented inline, too!
I hadn't intended to fix up the db_query() calls here, as there's already a patch for that here: #726344: Database system uses wrapper functions internally. I'll mark that duplicate, though, since it does clash with this patch on one line so I'm fine with doing it at the same time.
Everything here looks good except for the unit test. db_table_exists(), tb_index_exists(), and db_column_exists() are three separate operations so should get three separate test methods. (Some would argue 6 methods, one for each assert, but I won't go quite that far. :-) )
Once those tests get broken up, this is RTBC.
Comment #28
andypostI put all tests together because it seems too expensive to test each function with clean install, I thick we should keep tests are optimized too!
Comment #29
andypostRe-roll agianst changes #582948: Improve safety of schema manipulation
Also tests are separated back to different methods as Crell proposed.
Comment #30
andypostAnd patch
Comment #31
Crell CreditAttribution: Crell commentedThanks for carrying this home, andy!
Comment #32
Dries CreditAttribution: Dries commentedLooks great. Committed to CVS HEAD. Thanks.