I had another look at system.install and when populating the filter_format table Drupal also assumes ID 1 and 2 are assigned.

The attached patch moves the filter IDs to defines in include/bootstrap.inc and then explicitly uses the defined values when inserting the initial data.

I don't know if I should rewrite the filter_html_1 variable that is created to use the defines as well.

The rest of system_install() appears to be OK and not suffer from this problem.

The attached patches apply on top of the patches from issue #495956: system.install incorrectly assumes ANON and AUTH RID are always sequential.

CommentFileSizeAuthor
#17 filter-sequential-id-fix-5-D7.patch6.46 KBcafuego
#15 filter-sequential-id-fix-5-D7.patch6.46 KBcafuego
#7 filter-sequential-id-fix-2-D7.patch6.48 KBcafuego
#12 filter-sequential-id-fix-4-D7.patch6.56 KBcafuego
#10 filter-sequential-id-fix-3-D7.patch6.48 KBcafuego
#6 filter-sequential-id-fix-2-D6.patch3.04 KBcafuego
filter-sequential-rid-fix-D6.patch3.71 KBcafuego
filter-sequential-id-fix-D7.patch4.08 KBcafuego

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

As you change these INSERT queries, please convert them to DBTNG (in the D7 patch).

See http://drupal.org/node/310079. Actually, I already did the conversion in my patch at #491556: DBTNG the rest, so you can copy that and extend it with your changes.

damien tournoud’s picture

damien tournoud commented

The "1" value is used in filter_resolve_format() as a default filter if the 'filter_default_format' variable is not set.

Here is a roadmap:

- convert those query to DBTNG
- use the fid returned by the insert query to properly add the filters
- variable_set('filter_default_format',
)
- replace the ugly direct insertions to the variable table by variable_set()

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

http://api.drupal.org/api/constant/FILTER_FORMAT_DEFAULT/7 is defined in filter.module, apparently using 0 as value.

Damien's roadmap sounds sane.

@cafuego: The information about the special case in which this is happening is missing in your original post. You also might want to re-open the other issue for DBTNG conversion of the INSERT queries.

cafuego’s picture

cafuego commented

@sun yep, however FILTER_FORMAT_DEFAULT is a variable that is assigned a value from the database, at run-time... so either way the database needs to contain sane values that are identical to what Drupal expects them to be.

Rewriting this to DBTNG versions of inserts and (importantly) using the returned fids when adding the entries to the {filter} should sort it out, as per suggestion from Damien. I'll have a crack at that, but haven't used DBTNG before, so it may take some time (and many attempts).

cafuego’s picture

cafuego commented

Actually, I have a possible problem.

Every single call to variable_get('filter_default_format') assigns 1 if there is no value in the variables table.

Would it be permissible to store filter_default_format as the fid that is assigned by system.install, when system.install is processed?

Edit: never mind. PEBCAK

cafuego’s picture

cafuego commented
StatusFileSize
new filter-sequential-id-fix-2-D6.patch3.04 KB

Attached is a patch for D6 that should sort this. It also replaces the two INSERT statements for variables with variable_set().

Note, it applies over the top of the D6 patch for the RID id issue.

cafuego’s picture

cafuego commented
StatusFileSize
new filter-sequential-id-fix-2-D7.patch6.48 KB

Ok, here is the patch for D7.

I've re-used code from #491556: DBTNG the rest as suggested by Berdir. However, I have not modified the code that creates the first user account of anything that comes before it in system_install().

I've changed the INSERT INTO {variable} queries into variable_set() as was suggested on IRC.

Also on IRC, chk suggested that the filter_html_1 variable is no longer used and that I should remove it, so I have. See filter_update_7001().

cafuego’s picture

cafuego commented
Status: Needs work » Needs review

Changed status to needs review (forgot, oops)

Status: Needs review » Needs work

The last submitted patch failed testing.

cafuego’s picture

cafuego commented
Status: Needs work » Needs review
StatusFileSize
new filter-sequential-id-fix-3-D7.patch6.48 KB

Recreated patch against r1.347.

Status: Needs review » Needs work

The last submitted patch failed testing.

cafuego’s picture

cafuego commented
StatusFileSize
new filter-sequential-id-fix-4-D7.patch6.56 KB

Reworked the patch a little bit to (possibly) take advantage of bulk inserts. Tested and found working on local D7-HEAD, so this better not fail on the slave.

ugerhard’s picture

ugerhard commented
Status: Needs work » Needs review

Let it be tested.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

some minor things, I did not test the patch..

+  $query->values(array(
+    'rid' => DRUPAL_ANONYMOUS_RID,
+    'permission' => 'access content'
+  ));

For multi-line arrays, you need to add a , after the last element, in this case, after, 'access content'. This is also missing in a few other instances...

+  variable_set('theme_default', 's:7:"garland";');

variable_set will do the seralizing for you, you just need variable_set('theme_default', 'garland'). That is the advantage over using a direct insert query.

+  db_insert('filter')
//...
+    ->execute();
....
+  db_insert('filter')
//...
+    ->execute();

I think you can do all those inserts in a single query, just store the return of the first db_insert() into $query, remove the first execute and replace the second db_insert('filter') with $query.

cafuego’s picture

cafuego commented
StatusFileSize
new filter-sequential-id-fix-5-D7.patch6.46 KB

Ok, here goes nothing!

Status: Needs review » Needs work

The last submitted patch failed testing.

cafuego’s picture

cafuego commented
Status: Needs work » Needs review
StatusFileSize
new filter-sequential-id-fix-5-D7.patch6.46 KB

I can't find a problem. Re-test.

cafuego’s picture

cafuego commented
Status: Needs review » Closed (duplicate)

This patch is now rolled in to issue #495956: system.install incorrectly assumes ANON and AUTH RID are always sequential.